-
-
Notifications
You must be signed in to change notification settings - Fork 1.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)!: Compile-error on nested subcommands #2586
Conversation
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
Want to let you know that it will take me a bit of time to review this. |
Understandable. If we're good enough with the direction and just wanting to check details, I can take time to create an intermediate PR that moves functions to where they'll end up without any functionality changes, so the functionality changes stand out more. |
I would appreciate that. |
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
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
Waiting for rebase. Please only rebase this PR, no need to touch the other PRs until this is merged, that way we don't consume too much of the github CI and end up wasting time waiting for the CI queue. |
Rebased |
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.
Can you please add ui tests for the all the new errors we are sending?
src/derive.rs
Outdated
/// | ||
/// Implementing this trait let's a parent container delegate subcommand behavior to `Self`. | ||
/// with: | ||
/// - `#[clap(subcommand)] field: SubCmd`: Can be used with either struct fields or enum variants. |
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.
Can be used with either struct fields or enum variants.
What do you mean?
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.
Changing to
/// - `#[clap(subcommand)] field: SubCmd`: Attribute can be used with either struct fields or enum
/// variants that impl `Subcommand`.
/// - `#[clap(flatten)] Variant(SubCmd)`: Attribute can only be used with enum variants that impl
/// `Subcommand`.
Does that help?
src/derive.rs
Outdated
/// Implementing this trait let's a parent container delegate argument parsing behavior to `Self`. | ||
/// with: | ||
/// - `#[clap(flatten)] args: ChildArgs`: Can only be used with struct fields. | ||
/// - Implicitly: Can only be used with enum variants. |
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.
/// - Implicitly: Can only be used with enum variants. | |
/// - `Variant(ChildArgs)`: Can only be used with enum variants. |
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.
The descriptions here aren't clear enough.
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.
Changing to
/// - `#[clap(flatten)] args: ChildArgs`: Attribute can only be used with struct fields that impl
/// `Args`.
/// - `Variant(ChildArgs)`: No attribute is used with enum variants that impl `Args`.
src/derive.rs
Outdated
/// Implementing this trait let's a parent container delegate subcommand behavior to `Self`. | ||
/// with: | ||
/// - `#[clap(subcommand)] field: SubCmd`: Can be used with either struct fields or enum variants. | ||
/// - `#[clap(flatten)] Variant(SubCmd)`: Can only be used with enum variants. |
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.
The descriptions here aren't clear enough.
Before, partial command lines would panic at runtime. Now it'll be a compile error For example: ``` pub enum Opt { Daemon(DaemonCommand), } pub enum DaemonCommand { Start, Stop, } ``` Gives: ``` error[E0277]: the trait bound `DaemonCommand: clap::Args` is not satisfied --> clap_derive/tests/subcommands.rs:297:16 | 297 | Daemon(DaemonCommand), | ^^^^^^^^^^^^^ the trait `clap::Args` is not implemented for `DaemonCommand` | = note: required by `augment_args` ``` To nest this, you currently need `enum -> struct -> enum`. A later change will make it so you can use the `subcommand` attribute within enums to cover this case. This is a part of clap-rs#2005
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
I've updated everything, including adding ui tests. |
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
Before, partial command lines would panic at runtime. Now it'll be a
compile error
For example:
Gives:
To nest this, you currently need
enum -> struct -> enum
. A laterchange will make it so you can use the
subcommand
attribute withinenums to cover this case.
This is done by splitting out an
Args
trait fromIntoApp
, likeSubcommand
previously did in #1681.As a side benefit of this work, a user can now
derive(Args)
for astruct
while before they couldderive(IntoApp)
but must manually implementFromArgMatches
since no derive existed for it.Unlike with #1681, I did not move
from
/update
logic into the trait but moved all of that logic into theFromArgMatches
trait and it a supertrait forArgs
andSubcommand
, deriving them all together. For the top-level, we needFromArgMatches
, likeIntoApp
but unlikeIntoApp
, we don't need divergent APIs. AFromArgMatches
impl is also coupled to the augment impl.To support
FromArgMatches
being used in all cases, I had to switch its return type fromSelf
toOption<Self>
for when you have#[clap(subcommand)] cmd: Option<SubCmd>
. The caller has control over whether it is required or not.So parsing call graph looks like:
BREAKING CHANGES:
Depends on #2595
This is a part of #2005