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

ArgRequiredElseHelp is ignored if there are *any* default values #1264

Closed
whitfin opened this issue May 1, 2018 · 9 comments · Fixed by #3421
Closed

ArgRequiredElseHelp is ignored if there are *any* default values #1264

whitfin opened this issue May 1, 2018 · 9 comments · Fixed by #3421
Labels
A-validators Area: ArgMatches validation logi C-bug Category: Updating dependencies E-medium Call for participation: Experience needed to fix: Medium / intermediate
Milestone

Comments

@whitfin
Copy link

whitfin commented May 1, 2018

Rust Version

rustc 1.25.0 (84203cac6 2018-03-25)

Affected Version of clap

2.31.2

Expected Behavior Summary

If ArgRequiredElseHelp is enabled, you should see the help menu if an option is not provided correctly.

Actual Behavior Summary

If you have two arguments, and one has a default, this the help menu is not correctly printed.

Steps to Reproduce the issue

Run the code below in a test project via cargo run. It won't work (rightly so) because the "second" option is not provided - but you'll get an error instead of the help menu.

Do the same again with the default value removed from "first", and you'll then see a help menu. Regardless of the default value of "first", it should show the help menu both times.

App::new("")
    // package metadata from cargo
    .name(env!("CARGO_PKG_NAME"))
    .about(env!("CARGO_PKG_DESCRIPTION"))
    .version(env!("CARGO_PKG_VERSION"))

    // arguments and flag details
    .arg(Arg::with_name("second")
        .help("second option")
        .required(true))
    .arg(Arg::with_name("first")
        .help("first option")
        .default_value("something"))

    // settings required for parsing
    .setting(AppSettings::ArgRequiredElseHelp)
@whitfin
Copy link
Author

whitfin commented May 1, 2018

I think perhaps https://github.com/kbknapp/clap-rs/blob/master/src/app/validator.rs#L63 should use !reqs_validated instead of matcher.is_empty().

I dropped a PR in #1265; all tests pass still and the issue appears fixed. I'm not familiar enough to know if this is the best approach, so let me know of any feedback.

Edit: actually that PR appears to render the help menu too frequently now, so closed for the time being :)

@kbknapp
Copy link
Member

kbknapp commented Jun 5, 2018

Sorry for the long wait, I've been away for a few weeks with my day job.

while I agree it's not optimal, a default value is a value and therefore clap treats it as such. I'd possibly consider adding some sort of setting such as UserArgRequiredElseHelp but I'm not sure it'd quite be worth it as the "fix" for this is to simply not provide a default and use something like, matches.value_of("my-arg-that-needs-a-default").unwrap_or("default_value") and just "tell" the user in the help text manually that there is a default value (but to clap there isn't).

@kbknapp kbknapp closed this as completed Jun 5, 2018
@Kayvlim
Copy link

Kayvlim commented Jul 14, 2020

Bringing this back because I found this issue when searching before opening a new one.

Versions:
rustc 1.44.1 (c7087fe00 2020-06-17)
clap 3.0.0-beta.1

@kbknapp: if the "fix" is to call unwrap_or instead of using the mechanism already in place for default_values, this either defeats the purpose of this mechanism or the purpose of ArgRequiredElseHelp.

I believe AppSettings::ArgRequiredElseHelp is only relevant when we want to display the help to a user that did not write any arguments in the command line. This is the typical interaction for "let me see how this program works".
This behavior should not be dependent on whether there are arguments with default values. The user still did not type any argument.
I should also point out that even when no arguments are .required(true), ArgRequiredElseHelp does the right thing as long as no default_values are present.

Is there any use case where ArgRequiredElseHelp makes sense when an arg with a default_value is present?

It seems to me that clap's approach to default values is equating them with the user passing them in the command line (with the exception of occurrences_of returning zero), but what I am expecting is that is_present returns true and any attempt to retrieve the value (such as calling value_of) returns the default_value instead of None, with no other behavior being affected.

@pksunkara
Copy link
Member

@CreepySkeleton you were working on default stuff recently, right? Can you take a look at this last comment?

@CreepySkeleton
Copy link
Contributor

I find myself agreeing with @Kayvlim . Default values aren't ephemeral, they are very real values. Clap treats them in almost exactly the same way as any other value (barring conflict resolution which they are exempt from, or at least should have been).

Is there any use case where ArgRequiredElseHelp makes sense when an arg with a default_value is present?

That is a good question. My answer would be no, there aren't, and in this case we should panic if ArgRequiredElseHelp is set and any of the args has default_value.

@Kayvlim
Copy link

Kayvlim commented Jul 22, 2020

@CreepySkeleton I suppose you are only agreeing with how I think clap works, but not with how I think it should be changed?

I don't agree that ArgRequiredElseHelp should panic if any arg has a default_value. It's a perfectly legitimate use case:

If the user runs my application without arguments, I want them to see the help, but if they run it with the single required argument, I want the application to run with a sensible set of default values for the arguments they didn't pass (or "override"). I shouldn't have to unwrap_or every non-passed argument to accomplish this when a mechanism for default values is available just because I also want to display the help when no arguments have been passed.

So, I think ArgRequiredElseHelp should do exactly the same thing it would if there were no default_values whatsoever. This doesn't make default values ephemeral. Instead, it stops pretending they were passed by the user when they really weren't.

In other words, I don't see default values as values the user positively intended to pass to the application by omission. I see them as values the application needs to assume when the user hasn't said anything about them and None is not acceptable.

@CreepySkeleton
Copy link
Contributor

Well, let's see what we can do about that...

@dimo414
Copy link

dimo414 commented Feb 5, 2021

I was bitten by this issue myself while trying to add ArgRequiredElseHelp to a CLI. It's wholly unintuitive - both to the user and the developer - that the use of default values would be considered sufficient to bypass this check.

@pksunkara pksunkara added this to the 3.1 milestone Feb 5, 2021
@epage epage removed this from the 3.1 milestone Dec 9, 2021
@epage epage added A-validators Area: ArgMatches validation logi C-bug Category: Updating dependencies E-medium Call for participation: Experience needed to fix: Medium / intermediate labels Dec 9, 2021
@epage
Copy link
Member

epage commented Dec 9, 2021

See issues linked from #3076 for other default handling issues.

@epage epage added this to the 3.x milestone Jan 4, 2022
@epage epage modified the milestones: 3.x, 3.1 Feb 2, 2022
epage added a commit to epage/clap that referenced this issue Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-validators Area: ArgMatches validation logi C-bug Category: Updating dependencies E-medium Call for participation: Experience needed to fix: Medium / intermediate
Projects
None yet
7 participants