From 85ad452c9b1844d09fd5827c801fdcf14b562831 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 3 Aug 2022 14:48:50 -0500 Subject: [PATCH] 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 `,,...` 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. --- CHANGELOG.md | 1 + src/builder/arg.rs | 102 +------------------------------ src/builder/arg_settings.rs | 3 - src/builder/debug_asserts.rs | 1 - src/parser/arg_matcher.rs | 4 +- tests/builder/app_settings.rs | 1 - tests/builder/default_vals.rs | 10 ++- tests/builder/help.rs | 18 ++---- tests/builder/multiple_values.rs | 30 ++++----- tests/builder/opts.rs | 13 +--- 10 files changed, 28 insertions(+), 155 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 19245e22aa3..f7ae047bcb3 100644 --- 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 index 5de6d097fa0..9866b842a9a 100644 --- 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::("opt").unwrap().collect::>(), ["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::("opt").unwrap().collect::>(), ["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 { 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 index 5eb6f96b8f5..a3d55a217ba 100644 --- 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 index 377a2957ee1..54ab21382e6 100644 --- 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 index 2b9853427a9..e220bdfe6d9 100644 --- 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 index bdb259c5444..799607f7c83 100644 --- a/tests/builder/app_settings.rs +++ b/tests/builder/app_settings.rs @@ -1151,7 +1151,6 @@ fn aaos_opts_mult_req_delims() { arg!(--opt ... "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 index fdf157ed186..0d2888f0305 100644 --- 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 index 3b95da2a887..139e7c98fb1 100644 --- a/tests/builder/help.rs +++ b/tests/builder/help.rs @@ -1580,10 +1580,10 @@ Kevin K. tests stuff USAGE: - test --fake : + test --fake OPTIONS: - -f, --fake : some help + -f, --fake 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 : + test [OPTIONS] --fake OPTIONS: - -f, --fake : some help + -f, --fake 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 : --birthday-song --birthday-song-volume + test [OPTIONS] --fake --birthday-song --birthday-song-volume OPTIONS: - -f, --fake : some help + -f, --fake some help --style