-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Specify cmd arg names (metavars), et al. #3626
Conversation
to match dvc.org cmd ref
Codecov Report
@@ Coverage Diff @@
## master #3626 +/- ##
==========================================
- Coverage 90.89% 90.88% -0.02%
==========================================
Files 153 153
Lines 9571 9571
==========================================
- Hits 8700 8699 -1
- Misses 871 872 +1
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -53,6 +53,7 @@ def add_parser(subparsers, parent_parser): | |||
"-f", | |||
"--file", | |||
help="Specify name of the DVC-file this command will generate.", | |||
metavar="<filename>", |
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.
I'm not sure how this is better than -f FILE
. <>
look out of place here, and I don't really see the value, sorry.
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.
But ok, if you want to, sure, lets do it this way. Please remove "Draft" flag from your PR.
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.
I think, the value can be better seen here - https://dvc.org/doc/command-reference/run#run
check with what dvc run
outputs.
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.
Ruslan, 2 benefits (besides matching the docs which we already changed 😅):
- The existing param names aren't descriptive. FILE isn't the best example of this but e.g.
--outs OUTS
why repeat the option name when the param name can hint to what kind of argument is accepted. - We didn't like how upper case looked in the docs. Looks like we're yelling haha. We checked Git and noticed they use
<>
e.g. https://git-scm.com/docs/git-commit
"Type of metrics (json/tsv/htsv/csv/hcsv). " | ||
"Type of metrics (json/yaml). " |
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.
c5fef03
to
91647ca
Compare
❗ 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. If the CLI API is changed, I have updated tab completion scripts.
❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)
Thank you for the contribution - we'll try to review it as soon as possible. 🙏