Repository: clap-rs/clap
Pull request #1587 (2019-10-30 03:12:37)

add 'default_missing_value' configuration option
v3-compatible version of #1468.
Re-opened from #1507 (after upstream branch rename auto-close).

> RFC/WIP ~ this PR is a quick proof-of-concept, inviting commentary before being updated and fleshed out (documentation, tests, etc).
>
> For an OPTION taking optional values (ie, .min_values(0).require_equals(true)), currently any time that option is encountered multiple times, occurs is incremented, but no entry is made into the indices and vals arrays for options with no/missing value. "None" would seem like a good entry but the vals array contains just that, values, not Options, so that would break compatibility without any great upside. However, adding a default for missing values can be done without breaking anything else...
>
> This PR adds support to supply a value for the "None"/missing-value case (eg, for "--OPTION") via a default_missing_value() method. Notably, the supplied value to default_missing_value() may differ from the OPTION's default_value().

`clap` (v3) compiles and tests without error after this patch, but there is no specific testing of the feature as of yet (WIP).

I'm happy to put some more effort towards this (adding tests, documentation, etc) if it is a desired feature addition.

kbknapp: As mentioned in the other comments, I'd like to see the most work put towards how we make it clear the difference between default value and default missing value, as well as thinking through all the possible edge cases. Perhaps naming it `default_empty_value` or such would help. 

The one other area of concern is how this interacts with the conditional default values, or should it? I'm perfectly OK saying it doesn't.
rivy: `toml v0.5.4` is breaking the Travis build ... 
Alex doesn't believe that changing the minimum rust version is a "semantic" change?
Maybe lock `toml` to `v0.5.3`?
rivy: I can easily change the name from `default_missing_value` to `default_empty_value`.

I'm concerned a bit about the semantics...

Is `--color=""` (or `--color=`) an "empty" value? a null value?

I do think that (1) `--color=""` (aka `--color=`) and (2) `--color` are semantically distinct and should be treated as (1) `color` with an empty string value (ie, "") and (2) `color` with no value (which pulls in the `default_missing_value` in the PR). But I'm not really sure of the best terminology for differentiating the two cases.
kbknapp: > Is --color="" (or --color=) an "empty" value? a null value?
> 
> I do think that (1) --color="" (aka --color=) and (2) --color are semantically distinct and should be treated as (1) color with an empty string value (ie, "") and (2) color with no value (which pulls in the default_missing_value in the PR).

I agree and you make a great point. The best terminology I can think of is `--color=` or `--color=""` is explicitly an empty value. Whereas `--color` is a null value, I like your terminology. So having said that I'd retract my `default_empty_value` idea, being that `--color=` and `--color=""` are actually what clap calls empty values.
tshepang: I'd say `--color=` should mean exactly the same as `--color`, because a value is not given at all, and that's what @rivy seems to favor also.
rivy: @tshepang , no, this PR is actually exactly the opposite. This PR and IMO ...

- `--color=""` == `--color=''` == `--color=`, and means "use the empty string ('') as the value for the `color` option"
- `--color` should be semantically different, meaning "use the *default value* for the `color` option"
tshepang: Hm, maybe `--color=` should be disallowed then (runtime error). It's not obvious to me it should be an empty string, instead of null.
rivy: @tshepang , most shells parse and return `--foo=` as the resultant argument for *all* of `--foo=`, `--foo=''`, and `--foo=""`. These are supplied, and there is usually no reasonable way to differentiate at the level of the executable. So, generally, `--foo=` is / has to be interpreted as foo with an empty string argument.

> Notably, windows CMD is, as usual, an exception, but most option libraries for windows emulate the *nix standard.
pksunkara: @rivy Please rebase and add some tests and docs.
rivy: Will do ... in the middle of landing a large PR to `coreutils`, so it'll have to be next week or so ...
rivy: @pksunkara , @kbknapp , the code is now rebased onto 'master' and updated to use the newer clap-v3 internals. I've also added a few tests. I should have some documentation comments done within the next day or so...

Please review and let me know what revisions you'd like and whatever else needs to be done as prep for merge.
pksunkara: Can you also add a test case which gets empty value for `--color=` even with `default_missing_value` specified. (You probably need to say `TODO` and comment out the test case since empty values don't work yet correctly)
rivy: @pksunkara , @kbknapp , @CreepySkeleton , docs added; some polish done based on initial feedback.
pksunkara: Tests are still failing and you haven't addressed all my comments.
rivy: Doc and lint tests are now passing.
All comments addressed, if not completely resolved.
rivy: Changes made based on feedback.
I'll squash to 3 commits (add, tests, docs) after initial tests pass.
pksunkara: Looks good. I don't have any other comments.

Patch:
diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs
index 81de2030..e7cfad09 100644
--- a/src/build/arg/mod.rs
+++ b/src/build/arg/mod.rs
@@ -83,6 +83,7 @@ pub struct Arg<'help> {
     pub(crate) val_delim: Option<char>,
     pub(crate) default_vals: Vec<&'help OsStr>,
     pub(crate) default_vals_ifs: VecMap<(Id, Option<&'help OsStr>, &'help OsStr)>,
+    pub(crate) default_missing_vals: Vec<&'help OsStr>,
     pub(crate) env: Option<(&'help OsStr, Option<OsString>)>,
     pub(crate) terminator: Option<&'help str>,
     pub(crate) index: Option<u64>,
@@ -2299,6 +2300,111 @@ impl<'help> Arg<'help> {
         self
     }
 
+    /// Specifies a value for the argument when the argument is supplied and a value is required
+    /// but the value is *not* specified at runtime.
+    ///
+    /// This configuration option is often used to give the user a shortcut and allow them to
+    /// efficiently specify an option argument without requiring an explicitly value. The `--color`
+    /// argument is a common example. By, supplying an default, such as `default_missing_value("always")`,
+    /// the user can quickly just add `--color` to the command line to produce the desired color output.
+    ///
+    /// **NOTE:** using this configuration option requires the use of the `.min_values(0)` and the
+    /// `.require_equals(true)` configuration option. These are required in order to unambiguously
+    /// determine what, if any, value was supplied for the argument.
+    ///
+    /// # Examples
+    ///
+    /// Here is an implementation of the common POSIX style `--color` argument.
+    ///
+    /// ```rust
+    /// # use clap::{App, Arg};
+    ///
+    /// macro_rules! app {
+    ///     () => {{
+    ///         App::new("prog")
+    ///             .arg(Arg::new("color").long("color")
+    ///                 .value_name("WHEN")
+    ///                 .possible_values(&["always", "auto", "never"])
+    ///                 .default_value("auto")
+    ///                 .overrides_with("color")
+    ///                 .min_values(0)
+    ///                 .require_equals(true)
+    ///                 .default_missing_value("always")
+    ///                 .about("Specify WHEN to colorize output.")
+    ///             )
+    ///    }};
+    /// }
+    ///
+    /// let mut m;
+    ///
+    /// // first, we'll provide no arguments
+    ///
+    /// m  = app!().get_matches_from(vec![
+    ///         "prog"
+    ///     ]);
+    ///
+    /// assert_eq!(m.value_of("color"), Some("auto"));
+    /// assert!(m.is_present("color"));
+    /// assert_eq!(m.occurrences_of("color"), 0);
+    ///
+    /// // next, we'll provide a runtime value to override the default (as usually done).
+    ///
+    /// m  = app!().get_matches_from(vec![
+    ///         "prog", "--color=never"
+    ///     ]);
+    ///
+    /// assert_eq!(m.value_of("color"), Some("never"));
+    /// assert!(m.is_present("color"));
+    /// assert_eq!(m.occurrences_of("color"), 1);
+    ///
+    /// // finally, we will use the shortcut and only provide the argument without a value.
+    ///
+    /// m  = app!().get_matches_from(vec![
+    ///         "prog", "--color"
+    ///     ]);
+    ///
+    /// assert_eq!(m.value_of("color"), Some("always"));
+    /// assert!(m.is_present("color"));
+    /// assert_eq!(m.occurrences_of("color"), 1);
+    /// ```
+    /// [`ArgMatches::occurrences_of`]: ./struct.ArgMatches.html#method.occurrences_of
+    /// [`ArgMatches::value_of`]: ./struct.ArgMatches.html#method.value_of
+    /// [`Arg::takes_value(true)`]: ./struct.Arg.html#method.takes_value
+    /// [`ArgMatches::is_present`]: ./struct.ArgMatches.html#method.is_present
+    /// [`Arg::default_value`]: ./struct.Arg.html#method.default_value
+    #[inline]
+    pub fn default_missing_value(self, val: &'help str) -> Self {
+        self.default_missing_values_os(&[OsStr::new(val)])
+    }
+
+    /// Provides a default value in the exact same manner as [`Arg::default_missing_value`]
+    /// only using [`OsStr`]s instead.
+    /// [`Arg::default_value`]: ./struct.Arg.html#method.default_value
+    /// [`OsStr`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html
+    #[inline]
+    pub fn default_missing_value_os(self, val: &'help OsStr) -> Self {
+        self.default_missing_values_os(&[val])
+    }
+
+    /// Like [`Arg::default_missing_value'] but for args taking multiple values
+    /// [`Arg::default_value`]: ./struct.Arg.html#method.default_value
+    #[inline]
+    pub fn default_missing_values(self, vals: &[&'help str]) -> Self {
+        let vals_vec: Vec<_> = vals.iter().map(|val| OsStr::new(*val)).collect();
+        self.default_missing_values_os(&vals_vec[..])
+    }
+
+    /// Provides default values in the exact same manner as [`Arg::default_values`]
+    /// only using [`OsStr`]s instead.
+    /// [`Arg::default_values`]: ./struct.Arg.html#method.default_values
+    /// [`OsStr`]: https://doc.rust-lang.org/std/ffi/struct.OsStr.html
+    #[inline]
+    pub fn default_missing_values_os(mut self, vals: &[&'help OsStr]) -> Self {
+        self.set_mut(ArgSettings::TakesValue);
+        self.default_missing_vals = vals.to_vec();
+        self
+    }
+
     /// Specifies the value of the argument if `arg` has been used at runtime. If `val` is set to
     /// `None`, `arg` only needs to be present. If `val` is set to `"some-val"` then `arg` must be
     /// present at runtime **and** have the value `val`.
@@ -4174,6 +4280,7 @@ impl<'a> From<&'a Yaml> for Arg<'a> {
                 "default_value" => yaml_to_str!(a, v, default_value),
                 "default_value_if" => yaml_tuple3!(a, v, default_value_if),
                 "default_value_ifs" => yaml_tuple3!(a, v, default_value_if),
+                "default_missing_value" => yaml_to_str!(a, v, default_missing_value),
                 "env" => yaml_to_str!(a, v, env),
                 "value_names" => yaml_vec_or_str!(v, a, value_name),
                 "groups" => yaml_vec_or_str!(v, a, group),
@@ -4340,7 +4447,8 @@ impl<'help> fmt::Debug for Arg<'help> {
              aliases: {:?}, short_aliases: {:?}, possible_values: {:?}, value_names: {:?}, \
              number_of_values: {:?}, max_values: {:?}, min_values: {:?}, value_delimiter: {:?}, \
              default_value_ifs: {:?}, value_terminator: {:?}, display_order: {:?}, env: {:?}, \
-             unified_ord: {:?}, default_value: {:?}, validator: {}, validator_os: {} \
+             unified_ord: {:?}, default_value: {:?}, validator: {}, validator_os: {}, \
+             default_missing_value: {:?}, \
              }}",
             self.id,
             self.name,
@@ -4371,7 +4479,8 @@ impl<'help> fmt::Debug for Arg<'help> {
             self.unified_ord,
             self.default_vals,
             self.validator.as_ref().map_or("None", |_| "Some(Fn)"),
-            self.validator_os.as_ref().map_or("None", |_| "Some(Fn)")
+            self.validator_os.as_ref().map_or("None", |_| "Some(Fn)"),
+            self.default_missing_vals
         )
     }
 }
diff --git a/src/parse/parser.rs b/src/parse/parser.rs
index c2ab2eee..1e05593f 100644
--- a/src/parse/parser.rs
+++ b/src/parse/parser.rs
@@ -1278,6 +1278,18 @@ where
                 &*Usage::new(self).create_usage_with_title(&[]),
                 self.app.color(),
             )?);
+        } else if needs_eq && min_vals_zero {
+            debug!("None and requires equals, but min_vals == 0");
+            if !opt.default_missing_vals.is_empty() {
+                debug!("Parser::parse_opt: has default_missing_vals");
+                for val in &opt.default_missing_vals {
+                    debug!(
+                        "Parser::parse_opt: adding value from default_missing_values; val = {:?}",
+                        val
+                    );
+                    self.add_val_to_arg(opt, &ArgStr::new(val), matcher, ValueType::CommandLine)?;
+                }
+            };
         } else {
             debug!("None");
         }


Test patch:
diff --git a/tests/default_missing_vals.rs b/tests/default_missing_vals.rs
new file mode 100644
index 00000000..a352aa2f
--- /dev/null
+++ b/tests/default_missing_vals.rs
@@ -0,0 +1,110 @@
+use clap::{App, Arg};
+
+#[test]
+fn opt_missing() {
+    let r = App::new("df")
+        .arg(
+            Arg::new("color")
+                .long("color")
+                .default_value("auto")
+                .min_values(0)
+                .require_equals(true)
+                .default_missing_value("always"),
+        )
+        .try_get_matches_from(vec![""]);
+    assert!(r.is_ok());
+    let m = r.unwrap();
+    assert!(m.is_present("color"));
+    assert_eq!(m.value_of("color").unwrap(), "auto");
+    assert_eq!(m.occurrences_of("color"), 0);
+}
+
+#[test]
+fn opt_present_with_missing_value() {
+    let r = App::new("df")
+        .arg(
+            Arg::new("color")
+                .long("color")
+                .default_value("auto")
+                .min_values(0)
+                .require_equals(true)
+                .default_missing_value("always"),
+        )
+        .try_get_matches_from(vec!["", "--color"]);
+    assert!(r.is_ok());
+    let m = r.unwrap();
+    assert!(m.is_present("color"));
+    assert_eq!(m.value_of("color").unwrap(), "always");
+    assert_eq!(m.occurrences_of("color"), 1);
+}
+
+#[test]
+fn opt_present_with_value() {
+    let r = App::new("df")
+        .arg(
+            Arg::new("color")
+                .long("color")
+                .default_value("auto")
+                .min_values(0)
+                .require_equals(true)
+                .default_missing_value("always"),
+        )
+        .try_get_matches_from(vec!["", "--color=never"]);
+    assert!(r.is_ok());
+    let m = r.unwrap();
+    assert!(m.is_present("color"));
+    assert_eq!(m.value_of("color").unwrap(), "never");
+    assert_eq!(m.occurrences_of("color"), 1);
+}
+
+// ToDO: [2020-05-20; rivy] test currently fails as empty values are still a work-in-progress (see <https://github.com/clap-rs/clap/pull/1587#issuecomment-631648788>)
+// #[test]
+// fn opt_present_with_empty_value() {
+//     let r = App::new("df")
+//         .arg(Arg::new("color").long("color")
+//             .default_value("auto")
+//             .min_values(0)
+//             .require_equals(true)
+//             .default_missing_value("always")
+//         )
+//         .try_get_matches_from(vec!["", "--color="]);
+//     assert!(r.is_ok());
+//     let m = r.unwrap();
+//     assert!(m.is_present("color"));
+//     assert_eq!(m.value_of("color").unwrap(), "");
+//     assert_eq!(m.occurrences_of("color"), 1);
+// }
+
+//## `default_value`/`default_missing_value` non-interaction checks
+
+#[test]
+fn opt_default() {
+    // assert no change to usual argument handling when adding default_missing_value()
+    let r = App::new("app")
+        .arg(
+            Arg::from("-o [opt] 'some opt'")
+                .default_value("default")
+                .default_missing_value("default_missing"),
+        )
+        .try_get_matches_from(vec![""]);
+    assert!(r.is_ok());
+    let m = r.unwrap();
+    assert!(m.is_present("o"));
+    assert_eq!(m.value_of("o").unwrap(), "default");
+}
+
+#[test]
+fn opt_default_user_override() {
+    // assert no change to usual argument handling when adding default_missing_value()
+    let r = App::new("app")
+        .arg(
+            Arg::from("-o [opt] 'some opt'")
+                .default_value("default")
+                .default_missing_value("default_missing"),
+        )
+        .try_get_matches_from(vec!["", "-o=value"]);
+    assert!(r.is_ok());
+    let m = r.unwrap();
+    assert!(m.is_present("o"));
+    assert_eq!(m.value_of("o").unwrap(), "value");
+}