From 67adc4acf99058657d30d51e2060f0d2c9d24ad8 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 27 Jul 2022 20:23:11 -0500 Subject: [PATCH] fix(parser)!: Apply default_missing_value per occurrence This both simplifies the code and the model we present to the user, making more sense. There is room for further exploration of tying flag actions into this. --- CHANGELOG.md | 1 + src/parser/parser.rs | 92 +++++++-------------------- tests/builder/default_missing_vals.rs | 22 +++++++ 3 files changed, 46 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c14ae71f36a..eead80e826d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Disallow more `value_names` than `number_of_values` (#2695) - Replaced `cmd.allow_invalid_for_utf8_external_subcommands` with `cmd.external_subcommand_value_parser` (#3733) - Changed the default type of `allow_external_subcommands` from `String` to `OsString` +- `Arg::default_missing_value` now applies per occurrence rather than if a value is missing across all occurrences - *(assert)* Always enforce that version is specified when the `ArgAction::Version` is used - *(assert)* Add missing `#[track_caller]`s to make it easier to debug asserts - *(assert)* Ensure `overrides_with` IDs are valid diff --git a/src/parser/parser.rs b/src/parser/parser.rs index b3936dc8a7f..79cf4445af0 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -964,25 +964,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if arg.is_require_equals_set() && !has_eq { if arg.min_vals == Some(0) { debug!("Requires equals, but min_vals == 0"); - let mut arg_values = Vec::new(); - // We assume this case is valid: require equals, but min_vals == 0. - if !arg.default_missing_vals.is_empty() { - debug!("Parser::parse_opt_value: has default_missing_vals"); - for v in arg.default_missing_vals.iter() { - let trailing_values = false; // CLI should not be affecting default_missing_values - let _parse_result = self.split_arg_values( - arg, - &RawOsStr::new(v), - trailing_values, - &mut arg_values, - ); - if let Some(_parse_result) = _parse_result { - if _parse_result != ParseResult::ValuesDone { - debug!("Parser::parse_opt_value: Ignoring state {:?}; no values accepted after default_missing_values", _parse_result); - } - } - } - }; + let arg_values = Vec::new(); let react_result = self.react( Some(ident), ValueSource::CommandLine, @@ -1133,11 +1115,32 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { ident: Option, source: ValueSource, arg: &Arg<'help>, - raw_vals: Vec, + mut raw_vals: Vec, matcher: &mut ArgMatcher, ) -> ClapResult { self.resolve_pending(matcher)?; + if raw_vals.is_empty() { + // We assume this case is valid: require equals, but min_vals == 0. + if !arg.default_missing_vals.is_empty() { + debug!("Parser::react: has default_missing_vals"); + for v in arg.default_missing_vals.iter() { + let trailing_values = false; // CLI should not be affecting default_missing_values + let _parse_result = self.split_arg_values( + arg, + &RawOsStr::new(v), + trailing_values, + &mut raw_vals, + ); + if let Some(_parse_result) = _parse_result { + if _parse_result != ParseResult::ValuesDone { + debug!("Parser::parse_opt_value: Ignoring state {:?}; no values accepted after default_missing_values", _parse_result); + } + } + } + }; + } + debug!( "Parser::react action={:?}, identifier={:?}, source={:?}", arg.get_action(), @@ -1350,55 +1353,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { fn add_default_value(&self, arg: &Arg<'help>, matcher: &mut ArgMatcher) -> ClapResult<()> { let trailing_values = false; // defaults are independent of the commandline - if !arg.default_missing_vals.is_empty() { - debug!( - "Parser::add_default_value:iter:{}: has default missing vals", - arg.name - ); - match matcher.get(&arg.id) { - Some(ma) if ma.all_val_groups_empty() => { - debug!( - "Parser::add_default_value:iter:{}: has no user defined vals", - arg.name - ); - // The flag occurred, we just want to add the val groups - let mut arg_values = Vec::new(); - for v in arg.default_missing_vals.iter() { - let _parse_result = self.split_arg_values( - arg, - &RawOsStr::new(v), - trailing_values, - &mut arg_values, - ); - if let Some(_parse_result) = _parse_result { - if _parse_result != ParseResult::ValuesDone { - debug!("Parser::add_default_value: Ignoring state {:?}; defaults are outside of the parse loop", _parse_result); - } - } - } - self.start_custom_arg(matcher, arg, ValueSource::CommandLine); - self.push_arg_values(arg, arg_values, matcher)?; - } - None => { - debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name); - // do nothing - } - _ => { - debug!( - "Parser::add_default_value:iter:{}: has user defined vals", - arg.name - ); - // do nothing - } - } - } else { - debug!( - "Parser::add_default_value:iter:{}: doesn't have default missing vals", - arg.name - ); - // do nothing - } - if !arg.default_vals_ifs.is_empty() { debug!("Parser::add_default_value: has conditional defaults"); if !matcher.contains(&arg.id) { diff --git a/tests/builder/default_missing_vals.rs b/tests/builder/default_missing_vals.rs index 7f3b6c66e37..dad62f2e2bc 100644 --- a/tests/builder/default_missing_vals.rs +++ b/tests/builder/default_missing_vals.rs @@ -143,6 +143,28 @@ fn opt_default_user_override() { ); } +#[test] +fn default_missing_value_per_occurrence() { + // assert no change to usual argument handling when adding default_missing_value() + let r = Command::new("cmd") + .arg( + arg!(o: -o ... "some opt") + .min_values(0) + .default_value("default") + .default_missing_value("default_missing"), + ) + .try_get_matches_from(vec!["", "-o", "-o=value", "-o"]); + assert!(r.is_ok(), "{}", r.unwrap_err()); + let m = r.unwrap(); + assert_eq!( + m.get_many::("o") + .unwrap() + .map(|v| v.as_str()) + .collect::>(), + vec!["default_missing", "value", "default_missing"] + ); +} + #[test] #[allow(clippy::bool_assert_comparison)] fn default_missing_value_flag_value() {