From f76a867e3f16a4dbb7f9e73652bcd5d14723fe5a Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 28 Jul 2022 15:31:40 -0500 Subject: [PATCH 1/2] fix: Simplify flag parsing Multiple values can only happen from env variables when configured by the user, so let's not go out of our way to deal with it. --- src/parser/parser.rs | 53 +++++++++++++------------------------------- 1 file changed, 16 insertions(+), 37 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index a427497d50c..d23e91a6ec1 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1184,17 +1184,10 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(ParseResult::ValuesDone) } ArgAction::SetTrue => { - let raw_vals = match raw_vals.len() { - 0 => { - vec![OsString::from("true")] - } - 1 => raw_vals, - _ => { - debug!("Parser::react ignoring trailing values: {:?}", raw_vals); - let mut raw_vals = raw_vals; - raw_vals.resize(1, Default::default()); - raw_vals - } + let raw_vals = if raw_vals.is_empty() { + vec![OsString::from("true")] + } else { + raw_vals }; matcher.remove(&arg.id); @@ -1203,17 +1196,10 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(ParseResult::ValuesDone) } ArgAction::SetFalse => { - let raw_vals = match raw_vals.len() { - 0 => { - vec![OsString::from("false")] - } - 1 => raw_vals, - _ => { - debug!("Parser::react ignoring trailing values: {:?}", raw_vals); - let mut raw_vals = raw_vals; - raw_vals.resize(1, Default::default()); - raw_vals - } + let raw_vals = if raw_vals.is_empty() { + vec![OsString::from("false")] + } else { + raw_vals }; matcher.remove(&arg.id); @@ -1222,21 +1208,14 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { Ok(ParseResult::ValuesDone) } ArgAction::Count => { - let raw_vals = match raw_vals.len() { - 0 => { - let existing_value = *matcher - .get_one::(arg.get_id()) - .unwrap_or(&0); - let next_value = existing_value.saturating_add(1); - vec![OsString::from(next_value.to_string())] - } - 1 => raw_vals, - _ => { - debug!("Parser::react ignoring trailing values: {:?}", raw_vals); - let mut raw_vals = raw_vals; - raw_vals.resize(1, Default::default()); - raw_vals - } + let raw_vals = if raw_vals.is_empty() { + let existing_value = *matcher + .get_one::(arg.get_id()) + .unwrap_or(&0); + let next_value = existing_value.saturating_add(1); + vec![OsString::from(next_value.to_string())] + } else { + raw_vals }; matcher.remove(&arg.id); From 8e20782bfdfaa6d296e7f2254fbb870bb4617cf6 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 28 Jul 2022 15:41:05 -0500 Subject: [PATCH 2/2] fix(parser): Rely on `default_missing_value` for flag actions In the short term, this just provides a back door to custom actions. Longer term, we can explore a `SetConst` action that relies on this behavior. Really, `SetTrue` and `SetFalse` are shortcuts for such an action but shortcuts can be helpful for usability. Apparently, this also reduced `.text` size by 1k --- CHANGELOG.md | 1 + src/builder/action.rs | 12 ++++++++++++ src/builder/arg.rs | 5 +++++ 3 files changed, 18 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fcfea3ccb6..95f3adad7d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Removed `PartialEq` and `Eq` from `Command` - `number_of_values(0)` no longer implies `takes_value(true).multiple_values(true)` - `number_of_values(1)` no longer implies `multiple_values(true)` +- `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) - *(env)* Parse `--help` and `--version` like any `ArgAction::SetTrue` flag (#3776) diff --git a/src/builder/action.rs b/src/builder/action.rs index 8d755927327..7d011b7cea8 100644 --- a/src/builder/action.rs +++ b/src/builder/action.rs @@ -260,6 +260,18 @@ impl ArgAction { } } + pub(crate) fn default_missing_value(&self) -> Option<&'static std::ffi::OsStr> { + match self { + Self::Set => None, + Self::Append => None, + Self::SetTrue => Some(std::ffi::OsStr::new("true")), + Self::SetFalse => Some(std::ffi::OsStr::new("false")), + Self::Count => None, + Self::Help => None, + Self::Version => None, + } + } + pub(crate) fn default_value_parser(&self) -> Option { match self { Self::Set => None, diff --git a/src/builder/arg.rs b/src/builder/arg.rs index 83feb8020e2..6388fca851b 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -4303,6 +4303,11 @@ impl<'help> Arg<'help> { self.default_vals = vec![default_value]; } } + if let Some(default_value) = action.default_missing_value() { + if self.default_missing_vals.is_empty() { + self.default_missing_vals = vec![default_value]; + } + } if action.takes_values() { self.settings.set(ArgSettings::TakesValue); } else {