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

Use clap::ArgAction in true and false #3784

Merged
merged 2 commits into from
Aug 16, 2022
Merged

Conversation

jarkonik
Copy link
Contributor

@jarkonik jarkonik commented Aug 6, 2022

Closes #3769

@jarkonik jarkonik changed the title Use clap::ArgAction in true and false Use clap::ArgAction in true and false Aug 6, 2022
@jarkonik
Copy link
Contributor Author

jarkonik commented Aug 6, 2022

Seems like using .action(ArgAction::Help) on clap command builder cancels command conflict validation set with .exclusive(true). Probably should be fixed upstream.

@tertsdiepraam
Copy link
Member

Oh that's unfortunate. Could you make an issue in the clap repo with a minimal reproduction?

Copy link
Sponsor Contributor

@sylvestre sylvestre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not ready

@tertsdiepraam
Copy link
Member

On second thought, I wonder if exclusive is the right thing to do here. Maybe we should just check whether the the number of arguments (i.e. argc) is 2 and check for --help and --versiononly if that's the case.

@tertsdiepraam
Copy link
Member

Given the outcome of the issue on clap, I think my previous comment is the right way forward.

@jarkonik
Copy link
Contributor Author

I've reimplemented the behaviour using argument count instead of exclusive flag as suggested

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@tertsdiepraam tertsdiepraam dismissed sylvestre’s stale review August 16, 2022 08:50

Concern has been addressed

@tertsdiepraam tertsdiepraam merged commit 5ecabb8 into uutils:main Aug 16, 2022
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.

true and false can be simplified with clap::ArgAction
3 participants