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

required_unless produces error on a flag #533

Closed
joshtriplett opened this issue Jun 22, 2016 · 8 comments
Closed

required_unless produces error on a flag #533

joshtriplett opened this issue Jun 22, 2016 · 8 comments
Assignees
Labels
A-docs Area: documentation, including docs.rs, readme, examples, etc... A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations
Milestone

Comments

@joshtriplett
Copy link
Contributor

As the clap documentation notes, you can't make a flag required. However, it should be possible to mark a flag as .required_unless another flag or argument. Doing so currently produces a runtime panic.

You can work around this with another argument by marking the argument as .required_unless the flag instead. However, given two flags you can't.

(Alternatively, if this would make the require requirements more difficult to express, the documentation could instead explicitly note this case and point to another way to say "one or more of these are required".)

@kbknapp
Copy link
Member

kbknapp commented Jun 23, 2016

The thought process behind that is somewhat opinionated...and being that clap is becoming more heavily used I'm willing to discuss changing.


Currently flags can't be required because they're simply boolean values. If one should be required, then I would have thought the developer should "imply" it's use (perhaps with a note in the usage stating that it's implied in certain cases). IMO if flag --foo is required, then forcing the end user to type --foo unnecessary verbosity. There isn't any additional context passed to the developer/program by forcing them to do so.

The case against my thinking would be forcing the user to type --foo means they know that particular functionality/feature is being used.

I'm curious to know what others thoughts on the matter are?


If the above reasoning is correct, and there is a case where required_unless falls within that reasoning yet still causes a runtime panic...that's a bug. In which case I just need some clarity on this particular case 😄

@joshtriplett
Copy link
Contributor Author

joshtriplett commented Jun 23, 2016

I completely agree with the rationale for not allowing .required on a flag; please don't change that.

Here's my use case: I have a command similar to git rebase, which takes a flag --interactive (-i) and a positional argument onto. You can run rebase -i with no other arguments, or rebase some_commit, or rebase -i some_commit, but rebase without either -i or onto makes no sense as it would have nothing to do. So, I'd like to say "you must specify at least one of onto or --interactive.

I'd be happy with another way of doing this, such as a require_any method on ArgGroup, especially if that would give clap enough semantic information to give an error message like "you must specify at least one of --interactive/-i or base" (along with the help). Ideally the usage message will also reflect this by calling out the flags in that ArgGroup and not burying them in [FLAGS].

@kbknapp
Copy link
Member

kbknapp commented Jun 23, 2016

Ah, ok I understand, thanks! I think what I'll try to do is add a method ArgGroup::multiple(bool) which when combined with ArgGroup::required(true) does exactly like you're saying. Vice the normal ArgGroup::required(true) which makes one of them required, but only one and the rest are all mutually exclusive.

@kbknapp kbknapp added C-enhancement Category: Raise on the bar on expectations A-docs Area: documentation, including docs.rs, readme, examples, etc... D: intermediate A-parsing Area: Parser's logic and needs it changed somehow. and removed T: RFC / question labels Jun 23, 2016
@kbknapp kbknapp added this to the 2.7.0 milestone Jun 23, 2016
@kbknapp kbknapp self-assigned this Jun 23, 2016
kbknapp added a commit that referenced this issue Jun 23, 2016
…f the args

Unlike the previous ArgGroups, which made all args in the group mutually exclusive, one can now use
`ArgGroup::multiple(true)` which states, "At least one of the args must be used, and using more
than one is also OK."

Closes #533
@kbknapp
Copy link
Member

kbknapp commented Jun 23, 2016

This is implemented with #535

@homu homu closed this as completed in 33689ac Jun 23, 2016
@joshtriplett
Copy link
Contributor Author

@kbknapp Thanks for the fast fix! I'll test this today, including what the error message looks like for not providing one of the arguments.

Any plans for a clap release incorporating this? (I can pull clap from git for testing, but it'd be nice to have a released version on crates.io.)

@joshtriplett
Copy link
Contributor Author

@kbknapp Just tested this; works perfectly. Thanks!

@kbknapp
Copy link
Member

kbknapp commented Jun 24, 2016

@joshtriplett Once #376 #536 are merged I'll post the new version to crates.io. If #376 takes too much time I may just bump and publish prior. So in the next day or so I should have the new version out on crates.io

@kbknapp
Copy link
Member

kbknapp commented Jun 24, 2016

Add #539 #538 and #537 to that list 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation, including docs.rs, readme, examples, etc... A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants