fix!: Remove Arg::rwquire_value_delimiter

In clap v3, `require_value_delimiter` activated an alternative parse mode where - `multiple_values` meant "multiple values within a single arg" - `number_of_values` having no parse impact, only validation impact - `value_names` being delimited values For unbounded `number_of_values`, this is exactly what `value_delimiter` provides. The only value is if someone wanted `value_name` to be `<file1>,<file2>,...` which can be useful and we might look into adding back in. Alternatively, this could be used for cases like key-value pairs but that has issues like not allowing the delimiter in the value which might be ok in some cases but not others. We already instead document that people should instead use `ValueParser` for this case. In removing this, we remove points of confusion at how the different multiple values and delimited value calls interact with each other. I know I would set `require_value_delimiter(true).multiple_values(true)` when it turns out all I needed was `value_delimiter(',')`. This also reduces the API surface area which makes it easier to discover what features we do provide. While this isn't big, this is also yet another small step towards reducing binary size and compile times.
epage · Aug 4, 2022 · 85ad452 · 85ad452
1 parent 5d1ec08
commit 85ad452
Show file tree

Hide file tree

Showing 10 changed files with 28 additions and 155 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -31,6 +31,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Replace `Arg::max_values` (across all occurrences) with `Arg::num_args(1..=M)` (per occurrence)
 - Replace `Arg::multiple_values(true)` with `Arg::num_args(1..)` and `Arg::multiple_values(false)` with `Arg::num_args(0)`
 - Remove `Arg::use_value_delimiter` in favor of `Arg::value_delimiter`
+- Remove `Arg::require_value_delimiter`, either users could use `Arg::value_delimiter` or implement a custom parser with `TypedValueParser`
 - `ArgAction::SetTrue` and `ArgAction::SetFalse` now prioritize `Arg::default_missing_value` over their standard behavior
 - *(help)* Make `DeriveDisplayOrder` the default and removed the setting.  To sort help, set `next_display_order(None)` (#2808)
 - *(help)* Subcommand display order respects `Command::next_display_order` instead of `DeriveDisplayOrder` and using its own initial display order value (#2808)

diff --git a/src/builder/arg.rs b/src/builder/arg.rs
@@ -1409,88 +1409,6 @@ impl<'help> Arg<'help> {
         self.takes_value(true)
     }
 
-    /// Specifies that *multiple values* may only be set using the delimiter.
-    ///
-    /// This means if an option is encountered, and no delimiter is found, it is assumed that no
-    /// additional values for that option follow. This is unlike the default, where it is generally
-    /// assumed that more values will follow regardless of whether or not a delimiter is used.
-    ///
-    /// **NOTE:** It's a good idea to inform the user that use of a delimiter is required, either
-    /// through help text or other means.
-    ///
-    /// # Examples
-    ///
-    /// These examples demonstrate what happens when `require_delimiter(true)` is used. Notice
-    /// everything works in this first example, as we use a delimiter, as expected.
-    ///
-    /// ```rust
-    /// # use clap::{Command, Arg, ArgAction};
-    /// let delims = Command::new("prog")
-    ///     .arg(Arg::new("opt")
-    ///         .short('o')
-    ///         .action(ArgAction::Set)
-    ///         .value_delimiter(',')
-    ///         .require_value_delimiter(true)
-    ///         .num_args(1..))
-    ///     .get_matches_from(vec![
-    ///         "prog", "-o", "val1,val2,val3",
-    ///     ]);
-    ///
-    /// assert!(delims.contains_id("opt"));
-    /// assert_eq!(delims.get_many::<String>("opt").unwrap().collect::<Vec<_>>(), ["val1", "val2", "val3"]);
-    /// ```
-    ///
-    /// In this next example, we will *not* use a delimiter. Notice it's now an error.
-    ///
-    /// ```rust
-    /// # use clap::{Command, Arg, error::ErrorKind, ArgAction};
-    /// let res = Command::new("prog")
-    ///     .arg(Arg::new("opt")
-    ///         .short('o')
-    ///         .action(ArgAction::Set)
-    ///         .require_value_delimiter(true))
-    ///     .try_get_matches_from(vec![
-    ///         "prog", "-o", "val1", "val2", "val3",
-    ///     ]);
-    ///
-    /// assert!(res.is_err());
-    /// let err = res.unwrap_err();
-    /// assert_eq!(err.kind(), ErrorKind::UnknownArgument);
-    /// ```
-    ///
-    /// What's happening is `-o` is getting `val1`, and because delimiters are required yet none
-    /// were present, it stops parsing `-o`. At this point it reaches `val2` and because no
-    /// positional arguments have been defined, it's an error of an unexpected argument.
-    ///
-    /// In this final example, we contrast the above with `clap`'s default behavior where the above
-    /// is *not* an error.
-    ///
-    /// ```rust
-    /// # use clap::{Command, Arg, ArgAction};
-    /// let delims = Command::new("prog")
-    ///     .arg(Arg::new("opt")
-    ///         .short('o')
-    ///         .action(ArgAction::Set)
-    ///         .num_args(1..))
-    ///     .get_matches_from(vec![
-    ///         "prog", "-o", "val1", "val2", "val3",
-    ///     ]);
-    ///
-    /// assert!(delims.contains_id("opt"));
-    /// assert_eq!(delims.get_many::<String>("opt").unwrap().collect::<Vec<_>>(), ["val1", "val2", "val3"]);
-    /// ```
-    #[inline]
-    #[must_use]
-    pub fn require_value_delimiter(mut self, yes: bool) -> Self {
-        if yes {
-            self.val_delim.get_or_insert(',');
-            self.setting(ArgSettings::RequireDelimiter)
-                .takes_value(true)
-        } else {
-            self.unset_setting(ArgSettings::RequireDelimiter)
-        }
-    }
-
     /// Sentinel to **stop** parsing multiple values of a given argument.
     ///
     /// By default when
@@ -3989,11 +3907,6 @@ impl<'help> Arg<'help> {
         self.is_set(ArgSettings::HiddenLongHelp)
     }
 
-    /// Report whether [`Arg::require_value_delimiter`] is set
-    pub fn is_require_value_delimiter_set(&self) -> bool {
-        self.is_set(ArgSettings::RequireDelimiter)
-    }
-
     /// Report whether [`Arg::require_equals`] is set
     pub fn is_require_equals_set(&self) -> bool {
         self.is_set(ArgSettings::RequireEquals)
@@ -4098,12 +4011,7 @@ impl<'help> Arg<'help> {
     // Used for positionals when printing
     pub(crate) fn name_no_brackets(&self) -> Cow<str> {
         debug!("Arg::name_no_brackets:{}", self.name);
-        let delim = if self.is_require_value_delimiter_set() {
-            self.val_delim.expect(INTERNAL_ERROR_MSG)
-        } else {
-            ' '
-        }
-        .to_string();
+        let delim = " ";
         if !self.val_names.is_empty() {
             debug!("Arg::name_no_brackets: val_names={:#?}", self.val_names);
 
@@ -4268,13 +4176,7 @@ impl Default for ArgProvider {
 pub(crate) fn render_arg_val(arg: &Arg) -> String {
     let mut rendered = String::new();
 
-    let delim_storage;
-    let delim = if arg.is_require_value_delimiter_set() {
-        delim_storage = arg.val_delim.expect(INTERNAL_ERROR_MSG).to_string();
-        &delim_storage
-    } else {
-        " "
-    };
+    let delim = " ";
 
     let arg_name_storage;
     let val_names = if arg.val_names.is_empty() {

diff --git a/src/builder/arg_settings.rs b/src/builder/arg_settings.rs
@@ -33,7 +33,6 @@ pub(crate) enum ArgSettings {
     Hidden,
     TakesValue,
     NextLineHelp,
-    RequireDelimiter,
     HidePossibleValues,
     AllowHyphenValues,
     RequireEquals,
@@ -56,7 +55,6 @@ bitflags! {
         const HIDDEN           = 1 << 4;
         const TAKES_VAL        = 1 << 5;
         const NEXT_LINE_HELP   = 1 << 7;
-        const REQ_DELIM        = 1 << 9;
         const DELIM_NOT_SET    = 1 << 10;
         const HIDE_POS_VALS    = 1 << 11;
         const ALLOW_TAC_VALS   = 1 << 12;
@@ -83,7 +81,6 @@ impl_settings! { ArgSettings, ArgFlags,
     Hidden => Flags::HIDDEN,
     TakesValue => Flags::TAKES_VAL,
     NextLineHelp => Flags::NEXT_LINE_HELP,
-    RequireDelimiter => Flags::REQ_DELIM,
     HidePossibleValues => Flags::HIDE_POS_VALS,
     AllowHyphenValues => Flags::ALLOW_TAC_VALS,
     RequireEquals => Flags::REQUIRE_EQUALS,

diff --git a/src/builder/debug_asserts.rs b/src/builder/debug_asserts.rs
@@ -768,7 +768,6 @@ fn assert_arg_flags(arg: &Arg) {
         }
     }
 
-    checker!(is_require_value_delimiter_set requires is_takes_value_set);
     checker!(is_hide_possible_values_set requires is_takes_value_set);
     checker!(is_allow_hyphen_values_set requires is_takes_value_set);
     checker!(is_require_equals_set requires is_takes_value_set);

diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs
@@ -202,9 +202,7 @@ impl ArgMatcher {
             "ArgMatcher::needs_more_vals: o={}, pending={}",
             o.name, num_pending
         );
-        if num_pending == 1 && o.is_require_value_delimiter_set() {
-            false
-        } else if let Some(expected) = o.get_num_args() {
+        if let Some(expected) = o.get_num_args() {
             debug!(
                 "ArgMatcher::needs_more_vals: expected={}, actual={}",
                 expected, num_pending

diff --git a/tests/builder/app_settings.rs b/tests/builder/app_settings.rs
@@ -1151,7 +1151,6 @@ fn aaos_opts_mult_req_delims() {
             arg!(--opt <val> ... "some option")
                 .action(ArgAction::Set)
                 .value_delimiter(',')
-                .require_value_delimiter(true)
                 .action(ArgAction::Append),
         )
         .try_get_matches_from(vec!["", "--opt=some", "--opt=other", "--opt=one,two"]);

diff --git a/tests/builder/default_vals.rs b/tests/builder/default_vals.rs
@@ -818,15 +818,14 @@ fn invalid_default_values() {
 fn valid_delimited_default_values() {
     use clap::{Arg, Command};
 
-    let _ = Command::new("test")
+    Command::new("test")
         .arg(
             Arg::new("arg")
                 .value_parser(clap::value_parser!(u32))
                 .value_delimiter(',')
-                .require_value_delimiter(true)
                 .default_value("1,2,3"),
         )
-        .try_get_matches();
+        .debug_assert();
 }
 
 #[cfg(debug_assertions)]
@@ -835,15 +834,14 @@ fn valid_delimited_default_values() {
 fn invalid_delimited_default_values() {
     use clap::{Arg, Command};
 
-    let _ = Command::new("test")
+    Command::new("test")
         .arg(
             Arg::new("arg")
                 .value_parser(clap::value_parser!(u32))
                 .value_delimiter(',')
-                .require_value_delimiter(true)
                 .default_value("one,two"),
         )
-        .try_get_matches();
+        .debug_assert();
 }
 
 #[test]

diff --git a/tests/builder/help.rs b/tests/builder/help.rs
@@ -1580,10 +1580,10 @@ Kevin K.
 tests stuff
 
 USAGE:
-    test --fake <some>:<val>
+    test --fake <some> <val>
 
 OPTIONS:
-    -f, --fake <some>:<val>    some help
+    -f, --fake <some> <val>    some help
     -h, --help                 Print help information
     -V, --version              Print version information
 ";
@@ -1597,8 +1597,6 @@ OPTIONS:
                 .required(true)
                 .value_names(&["some", "val"])
                 .action(ArgAction::Set)
-                .value_delimiter(',')
-                .require_value_delimiter(true)
                 .value_delimiter(':'),
         );
 
@@ -1612,10 +1610,10 @@ Will M.
 does stuff
 
 USAGE:
-    test [OPTIONS] --fake <some>:<val>
+    test [OPTIONS] --fake <some> <val>
 
 OPTIONS:
-    -f, --fake <some>:<val>    some help
+    -f, --fake <some> <val>    some help
     -h, --help                 Print help information
     -V, --version              Print version information
 
@@ -1633,8 +1631,6 @@ NETWORKING:
                 .required(true)
                 .value_names(&["some", "val"])
                 .action(ArgAction::Set)
-                .value_delimiter(',')
-                .require_value_delimiter(true)
                 .value_delimiter(':'),
         )
         .next_help_heading(Some("NETWORKING"))
@@ -1655,10 +1651,10 @@ Will M.
 does stuff
 
 USAGE:
-    test [OPTIONS] --fake <some>:<val> --birthday-song <song> --birthday-song-volume <volume>
+    test [OPTIONS] --fake <some> <val> --birthday-song <song> --birthday-song-volume <volume>
 
 OPTIONS:
-    -f, --fake <some>:<val>    some help
+    -f, --fake <some> <val>    some help
         --style <style>        Choose musical style to play the song
     -s, --speed <SPEED>        How fast? [possible values: fast, slow]
     -h, --help                 Print help information
@@ -1686,8 +1682,6 @@ fn multiple_custom_help_headers() {
                 .required(true)
                 .value_names(&["some", "val"])
                 .action(ArgAction::Set)
-                .value_delimiter(',')
-                .require_value_delimiter(true)
                 .value_delimiter(':'),
         )
         .next_help_heading(Some("NETWORKING"))

diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs
@@ -837,9 +837,8 @@ fn req_delimiter_long() {
         .arg(
             Arg::new("option")
                 .long("option")
-                .num_args(1..)
-                .value_delimiter(',')
-                .require_value_delimiter(true),
+                .num_args(1)
+                .value_delimiter(','),
         )
         .arg(
             Arg::new("args")
@@ -875,9 +874,8 @@ fn req_delimiter_long_with_equal() {
         .arg(
             Arg::new("option")
                 .long("option")
-                .num_args(1..)
-                .value_delimiter(',')
-                .require_value_delimiter(true),
+                .num_args(1)
+                .value_delimiter(','),
         )
         .arg(
             Arg::new("args")
@@ -913,9 +911,8 @@ fn req_delimiter_short_with_space() {
         .arg(
             Arg::new("option")
                 .short('o')
-                .num_args(1..)
-                .value_delimiter(',')
-                .require_value_delimiter(true),
+                .num_args(1)
+                .value_delimiter(','),
         )
         .arg(
             Arg::new("args")
@@ -951,9 +948,8 @@ fn req_delimiter_short_with_no_space() {
         .arg(
             Arg::new("option")
                 .short('o')
-                .num_args(1..)
-                .value_delimiter(',')
-                .require_value_delimiter(true),
+                .num_args(1)
+                .value_delimiter(','),
         )
         .arg(
             Arg::new("args")
@@ -989,9 +985,8 @@ fn req_delimiter_short_with_equal() {
         .arg(
             Arg::new("option")
                 .short('o')
-                .num_args(1..)
-                .value_delimiter(',')
-                .require_value_delimiter(true),
+                .num_args(1)
+                .value_delimiter(','),
         )
         .arg(
             Arg::new("args")
@@ -1028,10 +1023,9 @@ fn req_delimiter_complex() {
             Arg::new("option")
                 .long("option")
                 .short('o')
-                .num_args(1..)
+                .num_args(1)
                 .action(ArgAction::Append)
-                .value_delimiter(',')
-                .require_value_delimiter(true),
+                .value_delimiter(','),
         )
         .arg(Arg::new("args").num_args(1..).index(1))
         .try_get_matches_from(vec![

diff --git a/tests/builder/opts.rs b/tests/builder/opts.rs
@@ -336,11 +336,7 @@ fn multiple_vals_pos_arg_equals() {
 #[test]
 fn require_delims_no_delim() {
     let r = Command::new("mvae")
-        .arg(
-            arg!(o: -o [opt] ... "some opt")
-                .value_delimiter(',')
-                .require_value_delimiter(true),
-        )
+        .arg(arg!(o: -o [opt] ... "some opt").value_delimiter(','))
         .arg(arg!([file] "some file"))
         .try_get_matches_from(vec!["mvae", "-o", "1", "2", "some"]);
     assert!(r.is_err());
@@ -351,12 +347,7 @@ fn require_delims_no_delim() {
 #[test]
 fn require_delims() {
     let r = Command::new("mvae")
-        .arg(
-            arg!(o: -o <opt> "some opt")
-                .num_args(1..)
-                .value_delimiter(',')
-                .require_value_delimiter(true),
-        )
+        .arg(arg!(o: -o <opt> "some opt").value_delimiter(','))
         .arg(arg!([file] "some file"))
         .try_get_matches_from(vec!["", "-o", "1,2", "some"]);
     assert!(r.is_ok(), "{}", r.unwrap_err());