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

ArgMathces::args_present behaviour with ArgAction::SetTrue #5860

Closed
2 tasks done
Vaider7 opened this issue Dec 25, 2024 · 8 comments · Fixed by #5908
Closed
2 tasks done

ArgMathces::args_present behaviour with ArgAction::SetTrue #5860

Vaider7 opened this issue Dec 25, 2024 · 8 comments · Fixed by #5908
Labels
C-bug Category: Updating dependencies

Comments

@Vaider7
Copy link

Vaider7 commented Dec 25, 2024

Please complete the following tasks

Rust Version

rustc 1.85.0-nightly

Clap Version

4.5.23

Minimal reproducible code

fn main() {
    let cli = Command::new("myapp").arg(
        Arg::new("show")
            .short('s')
            .long("show")
            .action(ArgAction::SetTrue),
    );
    let matches = cli.get_matches_from(["myapp"]);
    assert!(!matches.args_present());
}

This code panic with message

assertion failed: !matches.args_present()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

No args were provided, but args_present() returns true, which I think counterintuitive

Steps to reproduce the bug with the above code

cargo run

Actual Behaviour

matches.args_present() returns true

Expected Behaviour

matches.args_present() returns false if no arguments were provided despite Arg::new("show").action(ArgAction::SetTrue)

Additional Context

No response

Debug Output

No response

@Vaider7 Vaider7 added the C-bug Category: Updating dependencies label Dec 25, 2024
@epage
Copy link
Member

epage commented Dec 25, 2024

The documentation says "present on command line" but the behavior is if any ids are present, like contains_id
https://github.com/clap-rs/clap/blob/master/clap_builder%2Fsrc%2Fparser%2Fmatches%2Farg_matches.rs#L578

With SetTrue, false is a default which makes it so it contains the id.

The value_source is the defínitive way to know.

The question is whether we should change the docs or the behavior.

@Vaider7
Copy link
Author

Vaider7 commented Dec 25, 2024

Well, personally, I think it's very important to be able to check whether some args were provided via command line or not. If we change the docs, the behavior of args_present remains the same. And if so, args_present is not useful for any CLI setup, which uses action(ArgAction::SetTrue), because it will always return true regardless of whether args were provided or not. So my personal suggestion is that args_present should return presence of arguments despite any Command setup.

I'm not very familiar with clap yet, so I'm sorry if I'm saying nonsense things

@epage
Copy link
Member

epage commented Feb 5, 2025

In #3382, we added args_present() as a mirror to is_present(id).

In clap 3, flag's only set a value when passed in. With #3786, the new SetTrue and SetFalse actions started setting defaults. In these cases, we could no longer conflate "present in the structure" with "present on the command line".

In #3814, we renamed is_present(id) to contains_id(id) to better fit with the new actions.

We also have the IsPresent predicate which does check the ValueSource.

@Vaider7
Copy link
Author

Vaider7 commented Feb 7, 2025

I see, thank you for the explanation.

But I still deeply convinced that checking if "any arg presented on the command line" is important feature (at least for my purposes). IsPresent can be used for only one concrete arg, and if there was similar functionality for ArgMathces, that would be great and very useful

@epage
Copy link
Member

epage commented Feb 7, 2025

Note that wasn't necessarily a comment about what should be done for this issue but notes I took as I researched how we got here and how the term "present" is used. This is important background for understanding where to go from here.

For instance

  • The original intent was to communicate if arguments are present on the command-line which we are no longer upholding
  • The term "present" in the name matches that original intent
  • We've been in this situation for 2.5 years and no one has noticed until now

@Vaider7
Copy link
Author

Vaider7 commented Feb 7, 2025

I think for my initial purpose to check whether any argument was provided on the command line or not the next code can be used:

// no args provided
if std::env::args() == 1 {
    // do some stuff ...
}

But the current doc of args_presnt() states:

  • Check if any args were present on the command line

And I find it confusing, because it's not true it we use Arg::new("somearg").action(ArgAction::SetTrue)

So my suggestion is just change its documentation. For checking presence of arguments on the command line the above simple code can be used

@epage
Copy link
Member

epage commented Feb 10, 2025

I think for my initial purpose to check whether any argument was provided on the command line or not the next code can be used:

// no args provided
if std::env::args() == 1 {
// do some stuff ...
}

...

So my suggestion is just change its documentation. For checking presence of arguments on the command line the above simple code can be used

That works for the root command but not any subcommands.

If we changed the documentation, we'd need to also deprecate and make a new name available because present has a specific meaning in clap.

@epage
Copy link
Member

epage commented Feb 11, 2025

Doing a quick browse on github

epage added a commit to epage/clap that referenced this issue Feb 11, 2025
When we switched flags to `ArgAction::SetTrue`,
we overlooked updating `args_present`.
Because of the default value for `ArgAction::SetTrue`,
`args_present` will always report true when a flag is defined.

Looking over uses on github, this will fix a couple bugs but should
otherwise be unnoticeable.

Fixes clap-rs#5860
epage added a commit to epage/clap that referenced this issue Feb 11, 2025
When we switched flags to `ArgAction::SetTrue`,
we overlooked updating `args_present`.
Because of the default value for `ArgAction::SetTrue`,
`args_present` will always report true when a flag is defined.

I went with the trivial implementation for now.  We could proactively
track this but was unsure about correctness vs overhead and so I thought
I'd have those using it pay for it.

Looking over uses on github, this will fix a couple bugs but should
otherwise be unnoticeable.

Fixes clap-rs#5860
epage added a commit to epage/clap that referenced this issue Feb 11, 2025
When we switched flags to `ArgAction::SetTrue`,
we overlooked updating `args_present`.
Because of the default value for `ArgAction::SetTrue`,
`args_present` will always report true when a flag is defined.

I went with the trivial implementation for now.  We could proactively
track this but was unsure about correctness vs overhead and so I thought
I'd have those using it pay for it.

Looking over uses on github, this will fix a couple bugs but should
otherwise be unnoticeable.

Fixes clap-rs#5860
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Updating dependencies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants