Skip to content
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): Support SubcommandsNegateReqs #2820

Merged
merged 1 commit into from
Oct 6, 2021
Merged

Conversation

epage
Copy link
Member

@epage epage commented Oct 5, 2021

Before there was no way to make SubcommandsNegateReqs with
clap_derive because it required a required field with a sentinel value
for when the required part was negated. We blocked that.

This turned out simpler than I expected.

This came out of the discussion for #2255 but that issue is more
specifically about the panic, so not closing it.

Before there was no way to make `SubcommandsNegateReqs` with
`clap_derive` because it required a required field with a sentinel value
for when the required part was negated.  We blocked that.

This turned out simpler than I expected.

This came out of the discussion for clap-rs#2255 but that issue is more
specifically about the panic, so not closing it.
@kbknapp
Copy link
Member

kbknapp commented Oct 5, 2021

Do we want to go ahead and tackle the full issue in this PR? Cause this fix alone still panics in the case of #2255. Granted, as we discussed in that issue, the developer should use an Option + clap(required = true), but that is not exactly intuitive. And the panic message that is emitted doesn't point them in the right direction either (using the test case in the linked issue):

thread 'required_option_type' panicked at 'app should verify arg is required', clap_derive/tests/options.rs:346:18

We should probably change that to something generic-ish like arg 'X' should be of type Option<T>, and manually add #[clap(required = true)] due to conflicting attributes. This doesn't say what those conflicting attributes are, because I'm not sure we want to try and enumerate all cases or detect which one triggered it, but at least points them in the right direction.

@epage
Copy link
Member Author

epage commented Oct 5, 2021

Do we want to go ahead and tackle the full issue in this PR

I was wanting to keep the PR focused. Handling the panic involves changes unrelated to the ones I was making.

@epage epage merged commit 6fd3e0b into clap-rs:master Oct 6, 2021
@epage epage deleted the option branch October 6, 2021 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants