Requires and Conflicts should ignore default_value #3020
Conversation
tests/double_require.rs
Outdated
| @@ -25,7 +25,7 @@ static ONLY_C_ERROR: &str = "error: The following required arguments were not pr | |||
| -b | |||
|
|
|||
| USAGE: | |||
| prog [OPTIONS] -b -c | |||
| prog [OPTIONS] -c -b | |||
There was a problem hiding this comment.
The changes in this file are intentional because the earlier messages were wrong. We always show existing arguments first in other cases too.
pksunkara
force-pushed
the
unrolling_reqs
branch
from
18eef99 to
605a903
Compare
|
@pksunkara could you elaborate on the issue at hand along with the fix? It'll make reviewing the code and how it fits into the overall project much easier. I can understand if you have full context, but speaking for myself I don't 😄 |
|
Basically, we used a common function to validate requirements and then also to build the error message if requirements fail. But they should actually be two different functions since the logic inside validation is actually different to the one in the other. This issue surfaced because of #2985 and this PR is to separate out the logic for those functions. |
|
Hmm, this sounds like maybe it's a symptom of a larger issue. Is there an issue you can point to, or do we need to create one with all the details? I'd rather prioritize v3 issues, and if this is a long standing issue, or one in which it's more deeply rooted than addressing the surface I'd prefer to look at it that way. |
|
Yeah, the usage string generation (used in error messages) has been a long standing issue but we never discovered this specific part until #2985 was merged. Because of that, a regression in error message has been created. We have many usage string bugs that I tagged as 3.1 previously that I planned to make priority after v3 release. |
pksunkara
force-pushed
the
unrolling_reqs
branch
from
605a903 to
936d3e8
Compare
pksunkara
added
the
S-blocked
|
I updated this and we should be looking at it after 3.0 |
pksunkara
changed the title
|
@pksunkara could you rebase this when you have a chance? |
epage
added
S-waiting-on-author
While this doesn't help in the public API (yet), this at least clarifies the intent in the implementation. This is building towards clap-rs#3020.
For some errors, we use the unroll logic to get the list of required arguments. The usage then does the same, but without a matcher. This was causing the lists to not match. As a side effect, this fixed an ordering issue where we were putting the present arg after the not-present arg. I assume its because we ended up reporting the items twice but the first time is correctly ordered and gets precedence. This was split out of clap-rs#3020
This was split out of clap-rs#3020
Unrolling serves two distinct purposes but we muddied them together - Is `requires` satisfied for validation - Report what arguments are currently considered required for usage This was split out of clap-rs#3020
As expected, this was caused because of a bad abstraction.
Fixes #3076