cmd ref: command options impros and consistency #1029
Conversation
jorgeorpinel
added
type: enhancement
and reorder gc: before get
jorgeorpinel
temporarily deployed
to
dvc-landing-cmd-ref-opt-simrmt
There was a problem hiding this comment.
looks good! the only improvement can be made probably is to name these params to match some "user space terminology". Especially, --outs-no-cache OUTS_NO_CAHCE looks strange and not very meanigful ... it should be --outs-no-cache [path,] (list of locations), etc.
Wile I agree with you @shcheklein, these are the argument value names from command help outputs: $ dvc run -h
usage: dvc run [-h] [-q | -v] [-d DEPS] [-o OUTS] [-O OUTS_NO_CACHE]
[-m METRICS] [-M METRICS_NO_CACHE] [-f FILE] [-c CWD] [-w WDIR]
...Should I change all these both here and in a corresponding PR to the core repo? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Why would merge something that we want to change even in the core repo? It's better to just open the issue in the core repo first, decide on how (if we need any at all?) do we show the accepted values. Alternatively, we can analyze the best practices (e.g. Git itself, Heroku CLI, etc) - how do they name, show accepted values and start driving the change from the docs side. |
Because merging it is an improvement from the current published version, that matches the DVC version in prod. I checked the Git cmd ref and they do the same as this PR, except the value placeholders have nicer names (descriptive and not in ALL_CAPS) i.e. Heroku's case is quite different. They use the exact same value name as the option name, but assign with I think we should decide one Ill send it as a suggestion PR to core, so the engineering team can give feedback, I vote for Git's way e.g. WDYT @shcheklein ? |
so, this is my concern. Matching DVC != docs/readability improvement. The idea itself to show values is very valuable, it's just really really painful to see things like:
it's a lot of duplication, upper case makes it hard to read, etc If want to do this, I would really come up with a better (Heroku or Git - both are fine to me) approach, implement it and show this to the core team. |
|
OK I can fix it here and send a simultaneous PR to the core repo. I'm a bit concerned about conflicts with regular updates and this PR so it would be ideal to merge ASAP but let me fix this first... ⏳
We agree we want to do this right? |
yes. in this also, would not consider this p1. |
| dependencies can be specified like this: `-d data.csv -d process.py`. Usually, | ||
| each dependency is a file or a directory with data, or a code file, or a | ||
| configuration file. DVC also supports certain | ||
| - `-d <path>`, `--deps <path>` - specify a file or a directory the stage depends |
There was a problem hiding this comment.
just to check - can it be a comma-separated list?
There was a problem hiding this comment.
Not for run --deps, but it can be in other options. I think the only one is gc -p. It would be shown in the Synopsis though, not here in the Options section.
There was a problem hiding this comment.
yeah, that's right ... it's more about options that could be specified once vs those that could be specified multiple time ... not sure if we need a special syntax to highlight that
There was a problem hiding this comment.
Ah good question. I didn't even think about that but it seems like a relevant thing to note somewhere. Opened #1041
|
I approved this already, so feel free to merge! It's a good improvement. |
jorgeorpinel
changed the title
--file FILE