Merge pull request #2360 from ldm0/allow_empty

Allow empty value by default, introduce NoEquals Error

src/build/arg/debug_asserts.rs

@@ -67,7 +67,7 @@ fn assert_app_flags(arg: &Arg) {
         }
     }
 
-    checker!(AllowEmptyValues requires TakesValue);
+    checker!(NoEmptyValues requires TakesValue);
     checker!(RequireDelimiter requires TakesValue | UseValueDelimiter);
     checker!(HidePossibleValues requires TakesValue);
     checker!(AllowHyphenValues requires TakesValue);

src/build/arg/mod.rs

@@ -4160,10 +4160,11 @@ impl<'help> Arg<'help> {
         self.multiple_occurrences(multi).multiple_values(multi)
     }
 
-    /// Allows an argument to accept explicitly empty values. An empty value must be specified at
+    /// Don't allow an argument to accept explicitly empty values. An empty value
-    /// the command line with an explicit `""`, `''`, or `--option=`
+    /// must be specified at the command line with an explicit `""`, `''`, or
+    /// `--option=`
     ///
-    /// **NOTE:** By default empty values are *not* allowed
+    /// **NOTE:** By default empty values are allowed.
     ///
     /// **NOTE:** Setting this requires [`ArgSettings::TakesValue`].
     ///
@@ -4173,11 +4174,12 @@ impl<'help> Arg<'help> {
     /// # use clap::{App, Arg, ArgSettings};
     /// Arg::new("file")
     ///     .long("file")
-    ///     .setting(ArgSettings::TakesValue)
+    ///     .takes_value(true)
-    ///     .setting(ArgSettings::AllowEmptyValues)
+    ///     .forbid_empty_values(true)
     /// # ;
     /// ```
-    /// The default is to *not* allow empty values.
+    ///
+    /// The default is allowing empty values.
     ///
     /// ```rust
     /// # use clap::{App, Arg, ErrorKind, ArgSettings};
@@ -4185,24 +4187,7 @@ impl<'help> Arg<'help> {
     ///     .arg(Arg::new("cfg")
     ///         .long("config")
     ///         .short('v')
-    ///         .setting(ArgSettings::TakesValue))
+    ///         .takes_value(true))
-    ///     .try_get_matches_from(vec![
-    ///         "prog", "--config="
-    ///     ]);
-    ///
-    /// assert!(res.is_err());
-    /// assert_eq!(res.unwrap_err().kind, ErrorKind::EmptyValue);
-    /// ```
-    /// By adding this setting, we can allow empty values
-    ///
-    /// ```rust
-    /// # use clap::{App, Arg, ArgSettings};
-    /// let res = App::new("prog")
-    ///     .arg(Arg::new("cfg")
-    ///         .long("config")
-    ///         .short('v')
-    ///         .setting(ArgSettings::TakesValue)
-    ///         .setting(ArgSettings::AllowEmptyValues))
     ///     .try_get_matches_from(vec![
     ///         "prog", "--config="
     ///     ]);
@@ -4210,8 +4195,36 @@ impl<'help> Arg<'help> {
     /// assert!(res.is_ok());
     /// assert_eq!(res.unwrap().value_of("config"), None);
     /// ```
+    ///
+    /// By adding this setting, we can forbid empty values.
+    ///
+    /// ```rust
+    /// # use clap::{App, Arg, ErrorKind, ArgSettings};
+    /// let res = App::new("prog")
+    ///     .arg(Arg::new("cfg")
+    ///         .long("config")
+    ///         .short('v')
+    ///         .takes_value(true)
+    ///         .forbid_empty_values(true))
+    ///     .try_get_matches_from(vec![
+    ///         "prog", "--config="
+    ///     ]);
+    ///
+    /// assert!(res.is_err());
+    /// assert_eq!(res.unwrap_err().kind, ErrorKind::EmptyValue);
+    /// ```
     /// [`ArgSettings::TakesValue`]: ./enum.ArgSettings.html#variant.TakesValue
     #[inline]
+    pub fn forbid_empty_values(self, empty: bool) -> Self {
+        if empty {
+            self.setting(ArgSettings::NoEmptyValues)
+        } else {
+            self.unset_setting(ArgSettings::NoEmptyValues)
+        }
+    }
+
+    /// Placeholder documentation
+    #[inline]
     pub fn multiple_values(self, multi: bool) -> Self {
         if multi {
             self.setting(ArgSettings::MultipleValues)

src/build/arg/settings.rs

@@ -8,7 +8,7 @@ bitflags! {
     struct Flags: u32 {
         const REQUIRED         = 1;
         const MULTIPLE_OCC     = 1 << 1;
-        const EMPTY_VALS       = 1 << 2;
+        const NO_EMPTY_VALS    = 1 << 2;
         const GLOBAL           = 1 << 3;
         const HIDDEN           = 1 << 4;
         const TAKES_VAL        = 1 << 5;
@@ -39,7 +39,7 @@ impl_settings! { ArgSettings, ArgFlags,
     Required("required") => Flags::REQUIRED,
     MultipleOccurrences("multipleoccurrences") => Flags::MULTIPLE_OCC,
     MultipleValues("multiplevalues") => Flags::MULTIPLE_VALS,
-    AllowEmptyValues("allowemptyvalues") => Flags::EMPTY_VALS,
+    NoEmptyValues("noemptyvalues") => Flags::NO_EMPTY_VALS,
     Hidden("hidden") => Flags::HIDDEN,
     TakesValue("takesvalue") => Flags::TAKES_VAL,
     UseValueDelimiter("usevaluedelimiter") => Flags::USE_DELIM,
@@ -80,8 +80,8 @@ pub enum ArgSettings {
     MultipleValues,
     /// Allows an arg to appear multiple times
     MultipleOccurrences,
-    /// Allows an arg accept empty values such as `""`
+    /// Disallows an arg from accepting empty values such as `""`
-    AllowEmptyValues,
+    NoEmptyValues,
     /// Hides an arg from the help message
     Hidden,
     /// Allows an argument to take a value (such as `--option value`)
@@ -130,8 +130,8 @@ mod test {
             ArgSettings::AllowHyphenValues
         );
         assert_eq!(
-            "allowemptyvalues".parse::<ArgSettings>().unwrap(),
+            "noemptyvalues".parse::<ArgSettings>().unwrap(),
-            ArgSettings::AllowEmptyValues
+            ArgSettings::NoEmptyValues
         );
         assert_eq!(
             "hidepossiblevalues".parse::<ArgSettings>().unwrap(),

src/parse/errors.rs

@@ -116,6 +116,7 @@ pub enum ErrorKind {
     /// let res = App::new("prog")
     ///     .arg(Arg::new("color")
     ///          .setting(ArgSettings::TakesValue)
+    ///          .setting(ArgSettings::NoEmptyValues)
     ///          .long("color"))
     ///     .try_get_matches_from(vec!["prog", "--color="]);
     /// assert!(res.is_err());

src/parse/parser.rs

@@ -1153,7 +1153,7 @@ impl<'help, 'app> Parser<'help, 'app> {
         // has_eq: --flag=value
         let mut has_eq = false;
         let no_val = val.is_none();
-        let empty_vals = opt.is_set(ArgSettings::AllowEmptyValues);
+        let no_empty_vals = opt.is_set(ArgSettings::NoEmptyValues);
         let min_vals_zero = opt.min_vals.unwrap_or(1) == 0;
         let require_equals = opt.is_set(ArgSettings::RequireEquals);
 
@@ -1161,7 +1161,7 @@ impl<'help, 'app> Parser<'help, 'app> {
         if let Some(fv) = val {
             has_eq = fv.starts_with("=");
             let v = fv.trim_start_n_matches(1, b'=');
-            if !empty_vals && (v.is_empty() || (require_equals && !has_eq)) {
+            if no_empty_vals && (v.is_empty() || (require_equals && !has_eq)) {
                 debug!("Found Empty - Error");
                 return Err(ClapError::empty_value(
                     opt,

src/parse/validator.rs

@@ -125,9 +125,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
                     ));
                 }
             }
-            if !arg.is_set(ArgSettings::AllowEmptyValues)
+            if arg.is_set(ArgSettings::NoEmptyValues) && val.is_empty() && matcher.contains(&arg.id)
-                && val.is_empty()
-                && matcher.contains(&arg.id)
             {
                 debug!("Validator::validate_arg_values: illegal empty val found");
                 return Err(Error::empty_value(

tests/default_vals.rs

@@ -14,7 +14,11 @@ fn opts() {
 #[test]
 fn opt_without_value_fail() {
     let r = App::new("df")
-        .arg(Arg::from("-o [opt] 'some opt'").default_value("default"))
+        .arg(
+            Arg::from("-o [opt] 'some opt'")
+                .default_value("default")
+                .forbid_empty_values(true),
+        )
         .try_get_matches_from(vec!["", "-o"]);
     assert!(r.is_err());
     let err = r.unwrap_err();

tests/empty_values.rs

@@ -0,0 +1,129 @@
+mod utils;
+
+use clap::{App, Arg, ArgSettings, ErrorKind};
+
+#[test]
+fn empty_values() {
+    let m = App::new("config")
+        .arg(Arg::new("config").long("config").takes_value(true))
+        .get_matches_from(&["config", "--config", ""]);
+    assert_eq!(m.value_of("config"), Some(""));
+}
+
+#[test]
+fn empty_values_with_equals() {
+    let m = App::new("config")
+        .arg(Arg::new("config").long("config").takes_value(true))
+        .get_matches_from(&["config", "--config="]);
+    assert_eq!(m.value_of("config"), Some(""));
+
+    let m = App::new("config")
+        .arg(Arg::new("config").short('c').takes_value(true))
+        .get_matches_from(&["config", "-c="]);
+    assert_eq!(m.value_of("config"), Some(""))
+}
+
+#[test]
+fn no_empty_values() {
+    let m = App::new("config")
+        .arg(
+            Arg::new("config")
+                .long("config")
+                .takes_value(true)
+                .forbid_empty_values(true),
+        )
+        .try_get_matches_from(&["config", "--config", ""]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue);
+
+    let m = App::new("config")
+        .arg(
+            Arg::new("config")
+                .short('c')
+                .takes_value(true)
+                .forbid_empty_values(true),
+        )
+        .try_get_matches_from(&["config", "-c", ""]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue)
+}
+
+#[test]
+fn no_empty_values_with_equals() {
+    let m = App::new("config")
+        .arg(
+            Arg::new("config")
+                .long("config")
+                .takes_value(true)
+                .setting(ArgSettings::NoEmptyValues),
+        )
+        .try_get_matches_from(&["config", "--config="]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue);
+
+    let m = App::new("config")
+        .arg(
+            Arg::new("config")
+                .short('c')
+                .takes_value(true)
+                .setting(ArgSettings::NoEmptyValues),
+        )
+        .try_get_matches_from(&["config", "-c="]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue);
+}
+
+#[test]
+fn no_empty_values_without_equals() {
+    let m = App::new("config")
+        .arg(
+            Arg::new("config")
+                .long("config")
+                .takes_value(true)
+                .setting(ArgSettings::NoEmptyValues),
+        )
+        .try_get_matches_from(&["config", "--config"]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue);
+
+    let m = App::new("config")
+        .arg(
+            Arg::new("config")
+                .short('c')
+                .takes_value(true)
+                .setting(ArgSettings::NoEmptyValues),
+        )
+        .try_get_matches_from(&["config", "-c"]);
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::EmptyValue)
+}
+
+#[test]
+fn no_empty_values_without_equals_but_requires_equals() {
+    let app = App::new("config").arg(
+        Arg::new("config")
+            .long("config")
+            .takes_value(true)
+            .setting(ArgSettings::NoEmptyValues)
+            .setting(ArgSettings::RequireEquals),
+    );
+    let m = app.clone().try_get_matches_from(&["config", "--config"]);
+    // Should error on no equals rather than empty value.
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind, ErrorKind::NoEquals);
+
+    static NO_EUQALS_ERROR: &str =
+        "error: Equal sign is needed when assigning values to '--config=<config>'.
+
+USAGE:
+    config [OPTIONS]
+
+For more information try --help";
+
+    assert!(utils::compare_output(
+        app,
+        "config --config",
+        NO_EUQALS_ERROR,
+        true
+    ));
+}

tests/opts.rs

@@ -34,6 +34,7 @@ fn require_equals_fail() {
         .arg(
             Arg::new("cfg")
                 .setting(ArgSettings::RequireEquals)
+                .setting(ArgSettings::NoEmptyValues)
                 .setting(ArgSettings::TakesValue)
                 .long("config"),
         )
@@ -104,6 +105,7 @@ fn require_equals_no_empty_values_fail() {
             Arg::new("cfg")
                 .setting(ArgSettings::TakesValue)
                 .setting(ArgSettings::RequireEquals)
+                .setting(ArgSettings::NoEmptyValues)
                 .long("config"),
         )
         .arg(Arg::new("some"))
@@ -119,7 +121,6 @@ fn require_equals_empty_vals_pass() {
             Arg::new("cfg")
                 .setting(ArgSettings::TakesValue)
                 .setting(ArgSettings::RequireEquals)
-                .setting(ArgSettings::AllowEmptyValues)
                 .long("config"),
         )
         .try_get_matches_from(vec!["prog", "--config="]);
@@ -422,8 +423,10 @@ fn did_you_mean() {
 fn issue_665() {
     let res = App::new("tester")
         .arg("-v, --reroll-count=[N] 'Mark the patch series as PATCH vN'")
-        .arg(Arg::from(
+        .arg(
-"--subject-prefix [Subject-Prefix] 'Use [Subject-Prefix] instead of the standard [PATCH] prefix'") )
+            Arg::from("--subject-prefix [Subject-Prefix] 'Use [Subject-Prefix] instead of the standard [PATCH] prefix'")
+                .setting(ArgSettings::NoEmptyValues)
+        )
         .try_get_matches_from(vec!["test", "--subject-prefix", "-v", "2"]);
 
     assert!(res.is_err());
@@ -449,7 +452,7 @@ fn issue_1047_min_zero_vals_default_val() {
 
 fn issue_1105_setup(argv: Vec<&'static str>) -> Result<ArgMatches, clap::Error> {
     App::new("opts")
-        .arg(Arg::from("-o, --option [opt] 'some option'").setting(ArgSettings::AllowEmptyValues))
+        .arg(Arg::from("-o, --option [opt] 'some option'"))
         .arg(Arg::from("--flag 'some flag'"))
         .try_get_matches_from(argv)
 }