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

.action(ArgAction::Help) and ArgAction::Version bypass exclusive(true) #4046

Closed
2 tasks done
jarkonik opened this issue Aug 9, 2022 · 4 comments
Closed
2 tasks done
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies S-wont-fix Status: Closed as there is no plan to fix this

Comments

@jarkonik
Copy link

jarkonik commented Aug 9, 2022

Please complete the following tasks

Rust Version

rustc 1.62.1 (e092d0b6b 2022-07-16)

Clap Version

master

Minimal reproducible code

use clap::{error::ErrorKind, Arg, ArgAction, Command};

fn main() {
    let result = Command::new("flag_conflict")
        .version("5.0.0")
        .arg(
            Arg::new("myhelp")
                .action(ArgAction::Help)
                .long("myhelp")
                .exclusive(true),
        )
        .arg(
            Arg::new("myversion")
                .action(ArgAction::Version)
                .long("myversion"),
        )
        .try_get_matches_from(vec!["test", "--myhelp", "--myversion"]);
    assert_eq!(result.err().unwrap().kind(), ErrorKind::ArgumentConflict);
}

Steps to reproduce the bug with the above code

cargo run

Actual Behaviour

When using .action(ArgAction::Help) or .action(ArgAction::Version) and setting .exclusive(true) on one of them and when input contains both flags the returned error is DisplayError or DisplayVersion.

Expected Behaviour

When using .action(ArgAction::Help) or .action(ArgAction::Version) and setting .exclusive(true) on one of them and when input contains both flags it seems intuitive for the error to be ArgumentConflict

Additional Context

Would enable implementing false and true coreutils commands (uutils/coreutils#3784) using ArgAction::Version and ArgAction::Help

Debug Output

No response

@jarkonik jarkonik added the C-bug Category: Updating dependencies label Aug 9, 2022
@jarkonik jarkonik changed the title .action(ArgAction::Help) and ArgAction::Version bypass .action(ArgAction::Help) and ArgAction::Version bypass exclusive(true) Aug 9, 2022
@epage
Copy link
Member

epage commented Aug 9, 2022

It looks like you are taking manually implemented help and version flags and switching them to actions.

Could you expand on why those flags need to be exclusive?

@tertsdiepraam
Copy link
Contributor

Could you expand on why those flags need to be exclusive?

It's a weird behaviour of the GNU true and false utils. So a command like this

true --version

would show the version, but

true --version anotherarg

wouldn't. I believe the rationale is to maintain as much compatibility as possible with versions of the true and false commands that ignore all their arguments (like FreeBSD and built-ins of Bash).

We can work around this, but it was surprising to see the option being ignored when the ArgAction changed.

@epage
Copy link
Member

epage commented Aug 15, 2022

We can work around this, but it was surprising to see the option being ignored when the ArgAction changed.

ArgActions are specified in the documentation for how to react when evaluating an argument, as in they are immediately processed. An important aspect of this is that we want to make actions pluggable and would like toe avoid special casing as that is something the user can't replicate.

It's a weird behaviour of the GNU true and false utils.

exclusive(true) will cause an error which is the opposite of what you want here. To implement the GNU behavior, you would need to check if any other arg exists and ignore version/help if it does.

Considering the defined behavior of ArgAction, what seems like a relatively small use case (not even the described use case can rely on what is being requested, still small if it did), and the fact that its possible to workaround it, I'm going to go ahead and close this as part of our effort to limit the scope of clap to help keep binary sizes and compile times down

@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Aug 15, 2022
@epage epage added A-parsing Area: Parser's logic and needs it changed somehow. S-wont-fix Status: Closed as there is no plan to fix this labels Aug 15, 2022
@tertsdiepraam
Copy link
Contributor

tertsdiepraam commented Aug 15, 2022

Alright, we will work around it by checking the number of arguments. ArgAction being processed immediately seems counterintuitive, but I can see how it makes sense for counting etc.. Thank you!

I think the linked PR can then also be closed. @jarkonik Thank you for putting in the effort to implement this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies S-wont-fix Status: Closed as there is no plan to fix this
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants