Repository: clap-rs/clap (language: rust)

Issue:
Allow boolean literals as values for flags
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`.


Discussion / hints:
https://github.com/TeXitoi/structopt/issues/212#issuecomment-561093886
@emilk Could you please provide an exact snippet of code you're having the problem with? Along with expected/actual behavior.
Possible solution is to have a setting which allows flags to also allow boolean literals as values.
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.
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 
```rust
// 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");
```
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
#815 is related and might eliminate the need for boolean literals.
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 
@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?
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`)?
@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.
> @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 https://github.com/clap-rs/clap/blob/286fda0b4a243e19d73a254d2dc17c794f4320e5/tests/default_missing_vals.rs#L113-L141

I don't think it's necessary to do this in clap.
@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.
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.
@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.
> It's always nice to have some good defaults though, even as an external extension.

Fully agree.  Having better support for extending clap does not mean we leave it all in the users hands to build it themself.  That slipper slopes lead to there being no clap.
should this not be closed now that #2664 is merged?
> should this not be closed now that #2264 is merged?

from what i can tell, #2264 is a issue, not a PR and does not look related - do you mean #2664?
yes, was a typo sorry about that :sweat_smile: 
No, that PR only deals with environment variables while this issue is also about converting option values into boolean. With our new direction regarding plugin based architecture post v3, this is a prime candidate for being made a plugin. 
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
};
```
With clap 3.2, you can now override the bool behavior with `#[clap(action = ArgAction::Set)]`.

For example, from our test suite:
```rust
#[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 clap-rs/clap#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 clap-rs/clap#3832 for examples.


Fix patch:
diff --git a/src/builder/action.rs b/src/builder/action.rs
index 0e14b1c51f4..ab331429092 100644
--- a/src/builder/action.rs
+++ b/src/builder/action.rs
@@ -92,7 +92,8 @@ pub enum ArgAction {
     ///
     /// If no [`default_value`][super::Arg::default_value] is set, it will be `false`.
     ///
-    /// No value is allowed
+    /// No value is allowed. To optionally accept a value, see
+    /// [`Arg::default_missing_value`][super::Arg::default_missing_value]
     ///
     /// # Examples
     ///
@@ -127,7 +128,8 @@ pub enum ArgAction {
     ///
     /// If no [`default_value`][super::Arg::default_value] is set, it will be `true`.
     ///
-    /// No value is allowed
+    /// No value is allowed. To optionally accept a value, see
+    /// [`Arg::default_missing_value`][super::Arg::default_missing_value]
     ///
     /// # Examples
     ///
@@ -162,7 +164,8 @@ pub enum ArgAction {
     ///
     /// If no [`default_value`][super::Arg::default_value] is set, it will be `0`.
     ///
-    /// No value is allowed
+    /// No value is allowed. To optionally accept a value, see
+    /// [`Arg::default_missing_value`][super::Arg::default_missing_value]
     ///
     /// # Examples
     ///
diff --git a/src/builder/arg.rs b/src/builder/arg.rs
index 23ec1cb6e1a..63e88e80e48 100644
--- a/src/builder/arg.rs
+++ b/src/builder/arg.rs
@@ -2220,59 +2220,79 @@ impl<'help> Arg<'help> {
     ///
     /// # Examples
     ///
-    /// Here is an implementation of the common POSIX style `--color` argument.
-    ///
+    /// For POSIX style `--color`:
     /// ```rust
     /// # use clap::{Command, Arg, ValueSource};
-    ///
-    /// macro_rules! cmd {
-    ///     () => {{
-    ///         Command::new("prog")
-    ///             .arg(Arg::new("color").long("color")
-    ///                 .value_name("WHEN")
-    ///                 .value_parser(["always", "auto", "never"])
-    ///                 .default_value("auto")
-    ///                 .overrides_with("color")
-    ///                 .min_values(0)
-    ///                 .require_equals(true)
-    ///                 .default_missing_value("always")
-    ///                 .help("Specify WHEN to colorize output.")
-    ///             )
-    ///    }};
+    /// fn cli() -> Command<'static> {
+    ///     Command::new("prog")
+    ///         .arg(Arg::new("color").long("color")
+    ///             .value_name("WHEN")
+    ///             .value_parser(["always", "auto", "never"])
+    ///             .default_value("auto")
+    ///             .min_values(0)
+    ///             .require_equals(true)
+    ///             .default_missing_value("always")
+    ///             .help("Specify WHEN to colorize output.")
+    ///         )
     /// }
     ///
-    /// let mut m;
-    ///
     /// // first, we'll provide no arguments
-    ///
-    /// m  = cmd!().get_matches_from(vec![
+    /// let m  = cli().get_matches_from(vec![
     ///         "prog"
     ///     ]);
-    ///
     /// assert_eq!(m.value_of("color"), Some("auto"));
-    /// assert!(m.contains_id("color"));
     /// assert_eq!(m.value_source("color"), Some(ValueSource::DefaultValue));
     ///
     /// // next, we'll provide a runtime value to override the default (as usually done).
-    ///
-    /// m  = cmd!().get_matches_from(vec![
+    /// let m  = cli().get_matches_from(vec![
     ///         "prog", "--color=never"
     ///     ]);
-    ///
     /// assert_eq!(m.value_of("color"), Some("never"));
-    /// assert!(m.contains_id("color"));
     /// assert_eq!(m.value_source("color"), Some(ValueSource::CommandLine));
     ///
     /// // finally, we will use the shortcut and only provide the argument without a value.
-    ///
-    /// m  = cmd!().get_matches_from(vec![
+    /// let m  = cli().get_matches_from(vec![
     ///         "prog", "--color"
     ///     ]);
-    ///
     /// assert_eq!(m.value_of("color"), Some("always"));
-    /// assert!(m.contains_id("color"));
     /// assert_eq!(m.value_source("color"), Some(ValueSource::CommandLine));
     /// ```
+    ///
+    /// For bool literals:
+    /// ```rust
+    /// # use clap::{Command, Arg, ValueSource, value_parser};
+    /// fn cli() -> Command<'static> {
+    ///     Command::new("prog")
+    ///         .arg(Arg::new("create").long("create")
+    ///             .value_name("BOOL")
+    ///             .value_parser(value_parser!(bool))
+    ///             .min_values(0)
+    ///             .require_equals(true)
+    ///             .default_missing_value("true")
+    ///         )
+    /// }
+    ///
+    /// // first, we'll provide no arguments
+    /// let m  = cli().get_matches_from(vec![
+    ///         "prog"
+    ///     ]);
+    /// assert_eq!(m.get_one::<bool>("create").copied(), None);
+    ///
+    /// // next, we'll provide a runtime value to override the default (as usually done).
+    /// let m  = cli().get_matches_from(vec![
+    ///         "prog", "--create=false"
+    ///     ]);
+    /// assert_eq!(m.get_one::<bool>("create").copied(), Some(false));
+    /// assert_eq!(m.value_source("create"), Some(ValueSource::CommandLine));
+    ///
+    /// // finally, we will use the shortcut and only provide the argument without a value.
+    /// let m  = cli().get_matches_from(vec![
+    ///         "prog", "--create"
+    ///     ]);
+    /// assert_eq!(m.get_one::<bool>("create").copied(), Some(true));
+    /// assert_eq!(m.value_source("create"), Some(ValueSource::CommandLine));
+    /// ```
+    ///
     /// [`ArgMatches::value_of`]: ArgMatches::value_of()
     /// [`Arg::takes_value(true)`]: Arg::takes_value()
     /// [`Arg::default_value`]: Arg::default_value()


Test patch: