DOC: Auto-set metavar for parameters with value choices #3326
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Old output:
`--eval-subdataset-state EVAL_SUBDATASET_STATE`
New output:
`--eval-subdataset-state {'no', 'commit', 'full'}`
This improves the docs of a range of commands.
1 task
Codecov Report
@@ Coverage Diff @@
## master #3326 +/- ##
===========================================
+ Coverage 45.26% 91.11% +45.84%
===========================================
Files 260 263 +3
Lines 34209 34248 +39
===========================================
+ Hits 15486 31205 +15719
+ Misses 18723 3043 -15680
Continue to review full report at Codecov.
|
A nice '{v1|v2|...}' metavar value is now automatically generated. Use
it.
There are some spots where we can't switch to the automatically generated choice metavar because the constraints are compound rather than a simple EnsureChoice(). But let's use the same format for consistency.
When the parameter value is a set of choices that include None, we
define constraints in one of two ways:
EnsureNone() | EnsureChoice(...)
or
EnsureChoice(None, ...)
These behave the same, except that the display in the description is
slightly different and that the choice metavar can now be
automatically generated for the second case.
Switch most spots over to the second form to take advantage of the
automatically generated metavar. The one spot left unchanged is
sibling's action parameter because showing all the actions rather than
"ACTION" for the metavar seems less clear.
These parameters should be a set of choices but are defined with an open-ended EnsureStr(). Switch them to EnsureChoice() instead. Drop the explicit metavar values because these are now automatically generated for EnsureChoice() constraints.
|
@kyleam Lovely, thx much! |
kyleam
approved these changes
Apr 13, 2019
There was a problem hiding this comment.
Looks very nice, thanks.
I've pushed some commits to make wider use of this and to make the spots that don't use the automatically generated choice metavar still use a consistent style to format their choice metavar. Feel free to drop any changes you don't agree with.
yarikoptic
reviewed
Apr 13, 2019
|
Just an observation (probably not directly related to changes proposed
here)
Yeah, I'd say it's certainly orthogonal.
- so we have None, "none", and "" used across different commands options to represent "nothing", might be something to unify?
Hmm I'm not sure. None only works if it is being used as the default
value because it isn't accessible to Python callers. I think it'd be
too inconvenient and disruptive for all of the spots that look like
__call__(..., param=None) to become __call__(..., param="none"), so it
seems like we need both None and at least one string variant that means
"none". But I don't think we should collapse all empty string values to
"none" because "none" isn't always a good description (e.g., rerun's
--onto=).
On top of that, this is all user-facing, so we'd need deprecation
cycles. At first glance I don't think any effort to unify would be
worth it.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Old output:
--eval-subdataset-state EVAL_SUBDATASET_STATENew output:
--eval-subdataset-state {no|commit|full}This improves the docs of a range of commands, and addresses a comment by @kyleam in #3324