-
-
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
Test for Issue 1135 #1136
Test for Issue 1135 #1136
Conversation
It looks like required_unless_one has incorrect behavior if the argument it applies to also has a short form.
@willmurphyscode, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kbknapp to be a potential reviewer. |
Flags were incorrectly reporting that they never had required_unless args.
@@ -67,7 +67,7 @@ impl<'n, 'e> AnyArg<'n, 'e> for FlagBuilder<'n, 'e> { | |||
self.b.requires.as_ref().map(|o| &o[..]) | |||
} | |||
fn blacklist(&self) -> Option<&[&'e str]> { self.b.blacklist.as_ref().map(|o| &o[..]) } | |||
fn required_unless(&self) -> Option<&[&'e str]> { None } | |||
fn required_unless(&self) -> Option<&[&'e str]> { self.b.r_unless.as_ref().map(|o| &o[..]) } |
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.
This line is copied from the analogous line in the Positional arg builder: https://github.com/kbknapp/clap-rs/blob/f85fd9fbc8a1b2f5f319dad272405ac8e43c790f/src/args/arg_builder/positional.rs#L141 - I assume that the logic is supposed to be the same for both types of args.
Nice, good catch! Thanks @willmurphyscode 🎉 |
It looks like
required_unless_one
has incorrect behavior if theargument it applies to also has a short form. This adds a failing test, but I could use help fixing the issue.
This change is