New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
metrics: support TOML files #7965
Conversation
suffix = fs.path.suffix(path).lower() | ||
loader = LOADERS[suffix] | ||
val = loader(path, fs=fs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not blocker of this P.R. but I think we are repeating these same 3 lines in a few places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, It'd be nice to refactor this. But there's two main ways the suffix is obtained: fs.path.suffix(fs_path)
and os.path.splitext(fs_path)
. Is it safe to assume they are and always will be equivalent? Should one be preferred over the other?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to assume they are and always will be equivalent? Should one be preferred over the other?
I think it would always be the same. Anyhow, I think we can leave that out of this P.R.
It would be nice to update docs (i.e. https://dvc.org/doc/command-reference/metrics#supported-file-formats) |
Docs PR: #7965 |
@alexmojaki Could you |
Thanks @alexmojaki ! |
…7983) Following up on #7965 (comment) Co-authored-by: David de la Iglesia Castro <daviddelaiglesiacastro@gmail.com>
Fixes #6402
I haven't forbidden
.py
files as suggested there because it just seemed like it would clutter the code more than it would add any real value, but I can do so if requested.❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.