-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: Require validation not bypassed by mutually exclusive groups #6050
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
base: master
Are you sure you want to change the base?
Conversation
8bf8afa
to
4df478c
Compare
// If this argument is conditionally required (i.e., required by other present arguments | ||
// through the 'requires' relationship), it's NOT OK for it to be missing, even if it | ||
// conflicts with other arguments. However, directly required arguments (marked as | ||
// required(true)) can still be bypassed by conflicts. | ||
if self.required.contains(a.get_id()) && !a.is_required_set() { | ||
debug!("Validator::is_missing_required_ok: false (conditionally required)"); | ||
return false; | ||
} |
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.
While this might fix the immediate problem, I'm worried that this is too brittle and we can change things in other code and not realize this needs to be update as well.
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.
Note that #4520 is the core issue and it hasn't been accepted yet, requiring further analysis for correctness and whether this would constitute a breaking change.
For future contributions, please keep in mind
- We prefer to resolve issues before moving on to PRs
- Commits should be atomic, including tests passing
- We do ask for tests to be added in commits before the behavior changes but that is to show the existing behavior
- derive tests should go in the derive testsuite, not the builder one
- tests should be grouped by and named for their functionality, not issue numbers
See also https://github.com/clap-rs/clap/blob/master/CONTRIBUTING.md
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.
So wait, is 4707 not a workable issue? I have to wait until 4520 is resolved?
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.
They are the same concept (how should requires
interact with conflicts) and that question needs to be addressed before either moves forward. Rather than split that conversation between two related issues, I recommend we centralize it on #4520. That doesn't mean that #4520 needs to be implemented first. In fact, its likely that a fix for one will fix the other or be just a one or two line change.
Problem
Fixes #4707
Previously, arguments required via
requires
could be bypassed when they conflicted with other present arguments in mutually exclusive groups.Example Issue: