-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
fix(derive): Allow subcommands to directly nest in subcommands #2587
Merged
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
Until #2586 is landed, see the individual commit for reviewing whats relevant for this PR. |
epage
added a commit
to epage/clap
that referenced
this pull request
Jul 14, 2021
When debugging clap-rs#2586, I noticed we were developing match cases for variant names that were flattened. At minimum, this is dead code and at worst this could cause the wrong behavior if a user does an update with one of those names. Depends on clap-rs#2587 Fixes clap-rs#2588
The mac job got canceled for some reason |
epage
added a commit
to epage/clap
that referenced
this pull request
Jul 14, 2021
When debugging clap-rs#2586, I noticed we were developing match cases for variant names that were flattened. At minimum, this is dead code and at worst this could cause the wrong behavior if a user does an update with one of those names. Depends on clap-rs#2587 Fixes clap-rs#2588
epage
added a commit
to epage/clap
that referenced
this pull request
Jul 14, 2021
When debugging clap-rs#2586, I noticed we were developing match cases for variant names that were flattened. At minimum, this is dead code and at worst this could cause the wrong behavior if a user does an update with one of those names. Depends on clap-rs#2587 Fixes clap-rs#2588
epage
added a commit
to epage/clap
that referenced
this pull request
Jul 15, 2021
When debugging clap-rs#2586, I noticed we were developing match cases for variant names that were flattened. At minimum, this is dead code and at worst this could cause the wrong behavior if a user does an update with one of those names. Depends on clap-rs#2587 Fixes clap-rs#2588
`structopt` originally allowed ``` pub enum Opt { Daemon(DaemonCommand), } pub enum DaemonCommand { Start, Stop, } ``` This was partially broken in clap-rs#1681 where `$ cmd daemon start` works but `cmd daemon`, panics. Originally, `structopt` relied on exposing the implementation details of a derived type by providing a `is_subcommand` option, so we'd know whether to provide `SubcommandRequiredElseHelp` or not. This was removed in clap-rs#1681 Fixes clap-rs#2005
Rebased |
pksunkara
approved these changes
Jul 16, 2021
epage
added a commit
to epage/clap
that referenced
this pull request
Jul 16, 2021
When debugging clap-rs#2586, I noticed we were developing match cases for variant names that were flattened. At minimum, this is dead code and at worst this could cause the wrong behavior if a user does an update with one of those names. Depends on clap-rs#2587 Fixes clap-rs#2588
2 tasks
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.
structopt
originally allowedThis was partially broken in #1681 where
$ cmd daemon start
works butcmd daemon
,panics. Originally,
structopt
relied on exposing the implementationdetails of a derived type by providing a
is_subcommand
option, so we'dknow whether to provide
SubcommandRequiredElseHelp
or not. This wasremoved in #1681
With this change, the above code is now:
This can be taken a step further and support optional subcommands in variants like we do fields. That kind of consistency would help in people feeling confident in using the derives.
To implement this, I made it so we no longer re-used attribute validation between struct/enum and variants but instead we have variants broken out separately, like with fields. I tried to ensure the error validation was correct for struct/enum and variants. I think for struct/variant, we only want the default
Kind
, assumingFlatten
andExternalSubcommand
were allowed for variant sake. I didn't see whyFromGlobal
was allowed on struct/enum/variant, so I made it error in both cases.Depends on #2586
Fixes #2005