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(error): Don't duplicate args in usage #3689

Merged
merged 3 commits into from May 6, 2022
Merged

fix(error): Don't duplicate args in usage #3689

merged 3 commits into from May 6, 2022

Conversation

epage
Copy link
Member

@epage epage commented May 4, 2022

Gave up trying to decipher the existing logic for safe ways to
de-duplicate manually and switched to an IndexSet to enforce only one
of each argument exists.

Fixes #3556

Gave up trying to decipher the existing logic for safe ways to
de-duplicate manually and switched to an `IndexSet` to enforce only one
of each argument exists.

Fixes clap-rs#3556
epage added 2 commits May 5, 2022
This makes it consistent with other errors where we show arguments to
the user.
With us moving the required de-duplication up a level, it made this
check redundant.  By removing this check, we're more likely to have an
item in the `incls` which forces a smart usage and reduces the chance of
an `[ARGS]` or `[OPTIONS]`, so a couple of tests changed.
@epage epage merged commit dcda237 into clap-rs:master May 6, 2022
19 checks passed
@epage epage deleted the usage branch May 6, 2022
@leolovenet
Copy link

@leolovenet leolovenet commented May 6, 2022

By the way, there are two more required checks here, before calling create_usage_with_title, But it doesn't matter if you keep them the same

!(a.is_hide_set() || self.required.contains(&a.id))

.map_or(true, |a| !(required.contains(&a.id) || a.is_hide_set()))

epage added a commit to epage/clap that referenced this issue May 6, 2022
Made redundant in clap-rs#3689 but missed updating these.
@epage
Copy link
Member Author

@epage epage commented May 6, 2022

Thanks! I missed those

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