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

Allow boolean literals as values for flags #1649

Closed
emilk opened this issue Jan 31, 2020 · 30 comments · Fixed by #3837
Closed

Allow boolean literals as values for flags #1649

emilk opened this issue Jan 31, 2020 · 30 comments · Fixed by #3837
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing

Comments

@emilk
Copy link

emilk commented Jan 31, 2020

The behavior of --flag=false setting the flag to the value true has caused problems for me many times. The problem is that boolean arguments are treated differently than arguments of other types.

As an example, say you have a few arguments to your program being called from someplace like so:

my_app --name=${name} --age=${age} --alive=${alive}

The first two will work as expected, but the third will be a bug with no error or warning message. This special case for the boolean type is very annoying. I would advocate a more orthogonal design.

Here is my suggestion for how to parse boolean flags:

Argument Flag value
empty false
--flag true
--flag=true true
--flag=false false
--flag=other Helpful error message

The problem with this comes when considering how to treat other potential ways to express boolean, e.g. 0/1, no/yes, False/True. We could add these as well to the above table, or stick to the Rust convention (true/false). Either way would work for me.

In summary: the principle of least surprise suggests that if --int=42 should work the same as --bool=false.

@TeXitoi
Copy link
Contributor

TeXitoi commented Jan 31, 2020

@CreepySkeleton
Copy link
Contributor

@emilk Could you please provide an exact snippet of code you're having the problem with? Along with expected/actual behavior.

@pksunkara pksunkara changed the title --flag=false Allow boolean literals as values for flags Feb 1, 2020
@pksunkara
Copy link
Member

Possible solution is to have a setting which allows flags to also allow boolean literals as values.

@pksunkara pksunkara added this to the 3.1 milestone Feb 1, 2020
@CreepySkeleton CreepySkeleton added C: flags A-validators Area: ArgMatches validation logi S-waiting-on-decision Status: Waiting on a go/no-go before implementing and removed C: args labels Jul 7, 2020
@colemickens
Copy link

If there's a workaround someone could share, that would be great (I was wondering if maybe it's possible with the builder, or a custom parser? It seems like structopt has a way, but I can't find such functionality in clap at first glance).

To elaborate on justification for this bug... It is more than just surprising behavior. From what I can tell, it actively prevents certain designs. For example, I have a flag that really should be on by default (--create-users) and I don't want a --dont-create-users flag, I really want the user to have to say --create-users=false if they are expert enough to opt-out.

@CreepySkeleton
Copy link
Contributor

You couldn't find this functionality because it doesn't exist.

The key is that flags do not take value in clap, period. Flags that take value are called options. Options can be either present (possibly with a number of values following) or not. There's no notion of "if this option has one value and the value equals false then pretend that the option wasn't present".

structopt can do it because it applies postprocessing - conversion - to the values, and what you observe is result of the conversion. It doesn't change the fact that the option was actually present. You can simulate it with

// create an option with optional value
Arg::new("pseudo-flag")
    .possible_values(&["true", "false"])
    .long("foo")
    .min_values(0)
    .max_values(1)
    
/// ...

let setting = app.value_of("pseudo-flag") != Some("false");

@colemickens
Copy link

Thank you for the detailed explanation @CreepySkeleton, I hadn't quite understood the important distinction between "options" and "flags". It turns out, this also works, since I prefer explicit boolean options over "flags" -- https://github.com/clap-rs/clap/blob/dd5e6b23131f16ef8b090a8482f1b97c6a693498/clap_derive/examples/true_or_false.rs

@dimo414
Copy link

dimo414 commented Feb 5, 2021

#815 is related and might eliminate the need for boolean literals.

@ldm0
Copy link
Member

ldm0 commented Jul 31, 2021

I don't think clap needs to do something for this specific behaviour, you can get this behaviour by using default_missing_value and parsing later.

Here is the demo:
https://github.com/clap-rs/clap/pull/2649/files#diff-cf9f91bfd0a1de87f6e066071af5d05182179587e65ec540ed4cfba30ec1709bR113

@rami3l
Copy link
Contributor

rami3l commented Aug 1, 2021

@pksunkara Sorry, I changed my PR #2619 description to close another issue other than this, but I didn't reword the commit. Would you mind reopening this one?

@pksunkara pksunkara reopened this Aug 1, 2021
@epage
Copy link
Member

epage commented Aug 2, 2021

Now that #2619 is merged, we have an example for how to use clap's existing API for accomplishing the request in this Issue (taken taken from a previous comment).

Is this sufficient for resolving the need here or is there still interest in there being some baked in way of getting this behavior (possibly controllable with a ArgSetting / AppSetting)?

@rami3l
Copy link
Contributor

rami3l commented Aug 2, 2021

@epage In the end, no new examples has been added to the examples folder, so this remains to be a problem.

The thing that I'm considering now is that, for env vars it seems that we almost certainly have to include a string-literal-to-bool mechanism (which I'm currently working on) to replace the presence check, since it's quite a common practice.

Later on, if that's included, in my opinion it's better to do the same with flag literals for the sake of consistency. Of course, it would be better if this behavior is optional, and controlled by a new settings item.

Thus, the previously mentioned methods should only be considered as a workaround to satisfy some very specific needs.

@ldm0
Copy link
Member

ldm0 commented Aug 2, 2021

@epage In the end, no new examples has been added to the examples folder, so this remains to be a problem.

The thing that I'm considering now is that, for env vars it seems that we almost certainly have to include a string-literal-to-bool mechanism (which I'm currently working on) to replace the presence check, since it's quite a common practice.

Later on, if that's included, in my opinion it's better to do the same with flag literals for the sake of consistency. Of course, it would be better if this behavior is optional, and controlled by a new settings item.

Please check

fn default_missing_value_flag_value() {
let app = App::new("test").arg(
Arg::new("flag")
.long("flag")
.takes_value(true)
.default_missing_value("true"),
);
fn flag_value(m: ArgMatches) -> bool {
match m.value_of("flag") {
None => false,
Some(x) => x.parse().expect("non boolean value"),
}
}
assert_eq!(flag_value(app.clone().get_matches_from(&["test"])), false);
assert_eq!(
flag_value(app.clone().get_matches_from(&["test", "--flag"])),
true
);
assert_eq!(
flag_value(app.clone().get_matches_from(&["test", "--flag=true"])),
true
);
assert_eq!(
flag_value(app.clone().get_matches_from(&["test", "--flag=false"])),
false
);
}

I don't think it's necessary to do this in clap.

@rami3l
Copy link
Contributor

rami3l commented Aug 2, 2021

@ldm0 Thanks for your reference. Surely, I can see that this task can already be handled by the clap API with some handwritten validation functions.

I personally accept your solution, but the question that I really want to ask, is what level of API clap wants to provide for its users.

I wonder what a new user might guess about this library, and how we can guide them to get the solution that they want. That's all I care about. I chose a battery-included, opinionated approach, but you have the right to disagree, and as long as we keep being user-friendly, I'm satisfied with that outcome.

@epage
Copy link
Member

epage commented Aug 2, 2021

Sorry, missed that and hadn't caught up with your comment in #2539.

Personally, I see args and envs as separate entities that have their own context from a combination of history and user expectations. We should be consistent within their context rather than forcing a consistency between things users generally have treated differently.

env vars have historically been C-like in using 0/1 or present/not-present. More recently I feel like I've seen true/false used. Hence the need for a str2bool.

For arguments, I've generally seen one of

  • Present / not-present
  • --present, --no-present
  • --name <enumeration> with some being yes/no, true/false, always/never

Only in rare circumstances (--color) do I see any of these mixed. I've also not seen an argument parsing library that has baked in support (default or not) for mixing these without going custom (like the suggested workarounds)

As I've mentioned before, variable number of arguments has been brittle enough that clap warns about it. We can instead require an equals but I'm concerned about user confusion when some arguments need an = while others do not.

This issue and several others have been contributing a sense in me that we might want to re-think clap's API. Clap's development has been fairly generous in supporting everyone's use cases but the API is designed such that to do so, means it all has to be baked into clap, creating a large API surface and making it harder to get the behavior you need. After 3.0, I'm interested in exploring ways to allow people to customize the behavior of clap so that clap can be all things to all people without having to support all things out of the box.

@rami3l
Copy link
Contributor

rami3l commented Aug 2, 2021

@ldm0 So at least something should be added to the examples, whether it's the demo of a new feature or appropriate use of existing features. Since this is a decision that must be made carefully, it's always nice to have a discussion.

@epage Yeah, I also have the impression that the clap codebase is lacking appropriate reorganization. In this sense, I agree that we should make less stuff baked-in. It's always nice to have some good defaults though, even as an external extension.

@epage epage added A-parsing Area: Parser's logic and needs it changed somehow. and removed C: flags labels Dec 8, 2021
@epage epage removed this from the 3.1 milestone Dec 9, 2021
@epage epage added the C-enhancement Category: Raise on the bar on expectations label Dec 13, 2021
@bernardoaraujor
Copy link

bernardoaraujor commented Jun 15, 2022

the solution I found for this issue was to start using flag: Option<bool> instead of just flag: bool
that way, --flag=false gets the expected outcome and avoids confusion on the user's mind

downside is that I can't derive default_value (error: default_value is meaningless for Option ), so this workaround is also necessary:

let flag = match args.flag {
    Some(f) => f,
    None => false, // default
};

@epage
Copy link
Member

epage commented Jun 15, 2022

With clap 3.2, you can now override the bool behavior with #[clap(action = ArgAction::Set)].

For example, from our test suite:

#[test]
fn override_implicit_action() {
    #[derive(Parser, PartialEq, Debug)]
    struct Opt {
        #[clap(long, action = clap::ArgAction::Set)]
        arg: bool,
    }

    assert_eq!(
        Opt { arg: false },
        Opt::try_parse_from(&["test", "--arg", "false"]).unwrap()
    );

    assert_eq!(
        Opt { arg: true },
        Opt::try_parse_from(&["test", "--arg", "true"]).unwrap()
    );
}

If you had concerns with the rough upgrade to clap 3.2, the deprecations are now off by default, see #3830 for details. When you do start working through the deprecations as part of upgrading to 4.0, we've also improved the warnings for derive users. See #3832 for examples.

epage added a commit to epage/clap that referenced this issue Jun 15, 2022
Between
- `ArgAction::SetTrue` storing actual values
- `ArgAction::Set` making it easier for derive users to override bool
  behavior
- `Arg::default_missing_value` allowing hybrid-flags
- This commit documenting hybrid-flags even further

There shouldn't be anything left for clap-rs#1649

Fixes clap-rs#1649
@jbtobar
Copy link

jbtobar commented Oct 20, 2022

action = clap::ArgAction::Set

this example is incredibly useful. Should probably be better documented. I just spent too long digging through issues, documentation, debugging, etc to achieve the pretty basic --something=false functionality on the Parser macro.

Thank you!

@gibfahn
Copy link
Contributor

gibfahn commented Dec 1, 2023

I'm a bit confused by the resolution to this issue. I'm looking for something like the original post:

Argument Flag value
empty false
--flag true
--flag=true true
--flag=false false
--flag=other Helpful error message

Where the user can provide nothing (defaulting to true), or --flag=true or --flag=false, or not provide the flag at all (defaulting to either true or false), and the output is a bool. Anything else should error.

I can get this to work with Option<bool>:

    #[clap(
        long,
        default_missing_value = "true",
        require_equals = true,
        num_args(0..=1),
    )]
    pub(crate) flag: Option<bool>,

but I can't seem to work out how to get this to work with a plain bool, e.g.:

    #[clap(
        long,
        default_missing_value("true"),
        default_value("false"),
        num_args(0..=1),
        require_equals(true),
    )]
    pub(crate) flag: bool,

This fails with:

assertion `left == right` failed: Argument augment: mismatch between `num_args` (0..=1) and `takes_value`
  left: true
 right: false

Seems like there isn't a way to say "require equals or assume no argument passed".


EDIT: actually if you do this:

    #[clap(
        long,
        default_missing_value("true"),
        default_value("true"),
        num_args(0..=1),
        require_equals(true),
    )]
    pub(crate) flag: Option<bool>,

then you get the default behaviour I'm looking for, so the flag value will never be None, but rather Some(true) if not set. It's a bit odd to have a non-optional option, but it works well enough.

@epage
Copy link
Member

epage commented Dec 1, 2023

Try

    #[clap(
        long,
        default_missing_value("true"),
        default_value("true"),
        num_args(0..=1),
        require_equals(true),
        action = ArgAction::Set,
    )]
    pub(crate) flag: bool,

@gibfahn
Copy link
Contributor

gibfahn commented Dec 1, 2023

Ahh that works, thanks!

@JasonkayZK
Copy link

JasonkayZK commented Dec 2, 2023

Try

    #[clap(
        long,
        default_missing_value("true"),
        default_value("true"),
        num_args(0..=1),
        require_equals(true),
        action = ArgAction::Set,
    )]
    pub(crate) flag: bool,

Thanks, this works for me. 👍

And if you want default to be false, try this:

    #[clap(
        long,
        default_missing_value("true"),
        default_value("false"),
        num_args(0..=1),
        require_equals(true),
        action = ArgAction::Set,
    )]
    pub(crate) flag: bool,

paul-hansen added a commit to paul-hansen/sleek that referenced this issue Jan 20, 2024
Closes nrempel#16
Fixes the value of the uppercase option always being true.
This was due to the design of claps boolean flags which is
discussed here:
clap-rs/clap#1649 (comment)
This PR uses the solution mentioned in the linked comment from
that thread.

Using `require_equals` and `default_missing_value` allows
`sleek --uppercase "queries/*sql"` to still work without
trying to parse the path as the uppercase boolean, reducing
breakage.

Use `--uppercase=false` to disable uppercase, NOT `--uppercase false`.
paul-hansen added a commit to paul-hansen/sleek that referenced this issue Jan 20, 2024
Closes nrempel#16
Fixes the value of the uppercase option always being true.
This was due to the design of claps boolean flags which is
discussed here:
clap-rs/clap#1649 (comment)
This PR uses the solution mentioned in the linked comment from
that thread.

Using `require_equals` and `default_missing_value` allows
`sleek --uppercase "queries/*sql"` to still work without
trying to parse the path as the uppercase boolean, reducing
breakage.

Use `--uppercase=false` to disable uppercase, NOT `--uppercase false`.
@ctron
Copy link

ctron commented Jun 3, 2024

Is there a way to get this without require_equals?

@hasezoey
Copy link

hasezoey commented Jun 3, 2024

The following works for me for the variations below:

#[derive(Parser, Debug)]
pub struct CLI {
  #[arg(
        long = "test",
        action = ArgAction::Set,
        default_value_t = true,
        // somehow clap has this option not properly supported in derive, so it needs to be a string
        default_missing_value = "true",
        num_args = 0..=1,
        require_equals = false,
    )]
    pub test: bool,
}
$ binary
test: true
$ binary --test
test: true
$ binary --test=false
test: false
# the following would not be possible with `require_equals = true`
$ binary --test false
test: false
$ binary --test true
test: true

clap 4.5.3

@ctron
Copy link

ctron commented Jun 3, 2024

Thanks for the pointer. I got it working the way I wanted it with:

#[arg(long, default_missing_value="true", num_args=0..=1)]
pub offline: Option<bool>,

Which results in:

Args Value
None
--offline Some(true)
--offline, true Some(true)
--offline, false Some(false)

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-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Projects
None yet
Development

Successfully merging a pull request may close this issue.