From fb99d2c625bd808bdececb81968924dc23a19383 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 15:09:30 -0500 Subject: [PATCH 1/9] refactor(parser): Allow adding more actions in the future --- src/builder/action.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/builder/action.rs b/src/builder/action.rs index 3d1d1627030..4bfb59ebe76 100644 --- a/src/builder/action.rs +++ b/src/builder/action.rs @@ -1,5 +1,6 @@ /// Behavior of arguments when they are encountered while parsing #[derive(Clone, Debug, PartialEq, Eq)] +#[non_exhaustive] pub(crate) enum ArgAction { StoreValue, Flag, From eeca65369719469d1cd97a77faef6afa526d6944 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 15:10:07 -0500 Subject: [PATCH 2/9] refactor(parser): Loosen Action trait requirements --- src/builder/action.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/builder/action.rs b/src/builder/action.rs index 4bfb59ebe76..b9f716b3f5b 100644 --- a/src/builder/action.rs +++ b/src/builder/action.rs @@ -1,5 +1,5 @@ /// Behavior of arguments when they are encountered while parsing -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug)] #[non_exhaustive] pub(crate) enum ArgAction { StoreValue, From 7264bf28c7d6e2819388ed8a89cabaca1e873932 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 15:11:24 -0500 Subject: [PATCH 3/9] refactor(parser): Clarify Action::Flag's behavior --- src/builder/action.rs | 2 +- src/builder/command.rs | 2 +- src/parser/parser.rs | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/builder/action.rs b/src/builder/action.rs index b9f716b3f5b..fb819a505da 100644 --- a/src/builder/action.rs +++ b/src/builder/action.rs @@ -3,7 +3,7 @@ #[non_exhaustive] pub(crate) enum ArgAction { StoreValue, - Flag, + IncOccurrence, Help, Version, } diff --git a/src/builder/command.rs b/src/builder/command.rs index 8fbe5bc0622..10823c705df 100644 --- a/src/builder/command.rs +++ b/src/builder/command.rs @@ -4159,7 +4159,7 @@ impl<'help> App<'help> { let action = super::ArgAction::StoreValue; a.action = Some(action); } else { - let action = super::ArgAction::Flag; + let action = super::ArgAction::IncOccurrence; a.action = Some(action); } } diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 35a3501ddd8..2c958c2c2fe 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1167,7 +1167,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } Ok(ParseResult::ValuesDone) } - ArgAction::Flag => { + ArgAction::IncOccurrence => { debug_assert_eq!(raw_vals, Vec::::new()); if source == ValueSource::CommandLine { if matches!(ident, Some(Identifier::Short) | Some(Identifier::Long)) { @@ -1264,7 +1264,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } else { match arg.get_action() { ArgAction::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()), - ArgAction::Flag => { + ArgAction::IncOccurrence => { debug!("Parser::add_env: Found a flag with value `{:?}`", val); let predicate = str_to_bool(val.to_str_lossy()); debug!("Parser::add_env: Found boolean literal `{:?}`", predicate); From 2b9598516163b2fc69643c7d96b9c75595eb4080 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 15:17:21 -0500 Subject: [PATCH 4/9] refactor(parser): Ensure action and are are in-sync --- src/builder/action.rs | 11 +++++++++++ src/builder/debug_asserts.rs | 8 ++++++++ 2 files changed, 19 insertions(+) diff --git a/src/builder/action.rs b/src/builder/action.rs index fb819a505da..ee4f86bdc13 100644 --- a/src/builder/action.rs +++ b/src/builder/action.rs @@ -7,3 +7,14 @@ pub(crate) enum ArgAction { Help, Version, } + +impl ArgAction { + pub(crate) fn takes_value(&self) -> bool { + match self { + Self::StoreValue => true, + Self::IncOccurrence => false, + Self::Help => false, + Self::Version => false, + } + } +} diff --git a/src/builder/debug_asserts.rs b/src/builder/debug_asserts.rs index 9ffb4e9dfa6..d0287ed3047 100644 --- a/src/builder/debug_asserts.rs +++ b/src/builder/debug_asserts.rs @@ -642,6 +642,14 @@ fn assert_arg(arg: &Arg) { arg.name, ); + assert_eq!( + arg.get_action().takes_value(), + arg.is_takes_value_set(), + "Argument `{}`'s selected action {:?} contradicts `takes_value`", + arg.name, + arg.get_action() + ); + if arg.get_value_hint() != ValueHint::Unknown { assert!( arg.is_takes_value_set(), From b1b5820cb424fc89d7fd5797fa34994652f4ad2f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 15:27:15 -0500 Subject: [PATCH 5/9] refactor(asserts): Ensure we always check positionals take values --- src/builder/debug_asserts.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/builder/debug_asserts.rs b/src/builder/debug_asserts.rs index d0287ed3047..327887c4ed4 100644 --- a/src/builder/debug_asserts.rs +++ b/src/builder/debug_asserts.rs @@ -672,6 +672,11 @@ fn assert_arg(arg: &Arg) { "Argument '{}' is a positional argument and can't have short or long name versions", arg.name ); + assert!( + arg.is_takes_value_set(), + "Argument '{}` is positional, it must take a value", + arg.name + ); } if arg.is_required_set() { From 91480de6d2eaf3cdbe5ae743058a69cfc5b1a31d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 15:55:37 -0500 Subject: [PATCH 6/9] feat(builder): Expose ArgAction --- src/builder/action.rs | 118 +++++++++++++++++++++++++++++++++++++++++- src/builder/arg.rs | 32 +++++++++++- src/builder/mod.rs | 2 +- 3 files changed, 149 insertions(+), 3 deletions(-) diff --git a/src/builder/action.rs b/src/builder/action.rs index ee4f86bdc13..f83e62d151b 100644 --- a/src/builder/action.rs +++ b/src/builder/action.rs @@ -1,10 +1,126 @@ /// Behavior of arguments when they are encountered while parsing +/// +/// # Examples +/// +/// ```rust +/// # use clap::Command; +/// # use clap::Arg; +/// let cmd = Command::new("mycmd") +/// .arg( +/// Arg::new("special-help") +/// .short('?') +/// .action(clap::builder::ArgAction::Help) +/// ); +/// +/// // Existing help still exists +/// let err = cmd.clone().try_get_matches_from(["mycmd", "-h"]).unwrap_err(); +/// assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp); +/// +/// // New help available +/// let err = cmd.try_get_matches_from(["mycmd", "-?"]).unwrap_err(); +/// assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp); +/// ``` #[derive(Clone, Debug)] #[non_exhaustive] -pub(crate) enum ArgAction { +#[allow(missing_copy_implementations)] // In the future, we may accept `Box` +pub enum ArgAction { + /// When encountered, store the associated value(s) in [`ArgMatches`][crate::ArgMatches] + /// + /// # Examples + /// + /// ```rust + /// # use clap::Command; + /// # use clap::Arg; + /// let cmd = Command::new("mycmd") + /// .arg( + /// Arg::new("flag") + /// .long("flag") + /// .takes_value(true) + /// .action(clap::builder::ArgAction::StoreValue) + /// ); + /// + /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "value"]).unwrap(); + /// assert!(matches.is_present("flag")); + /// assert_eq!(matches.occurrences_of("flag"), 1); + /// assert_eq!( + /// matches.get_many::("flag").unwrap_or_default().map(|v| v.as_str()).collect::>(), + /// vec!["value"] + /// ); + /// ``` StoreValue, + /// When encountered, increment [`ArgMatches::occurrences_of`][crate::ArgMatches::occurrences_of] + /// + /// No value is allowed + /// + /// # Examples + /// + /// ```rust + /// # use clap::Command; + /// # use clap::Arg; + /// let cmd = Command::new("mycmd") + /// .arg( + /// Arg::new("flag") + /// .long("flag") + /// .multiple_occurrences(true) + /// .action(clap::builder::ArgAction::IncOccurrence) + /// ); + /// + /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); + /// assert!(matches.is_present("flag")); + /// assert_eq!(matches.occurrences_of("flag"), 2); + /// assert_eq!(matches.get_many::("flag").unwrap_or_default().count(), 0); + /// ``` IncOccurrence, + /// When encountered, display [`Command::print_help`][super::App::print_help] + /// + /// Depending on the flag, [`Command::print_long_help`][super::App::print_long_help] may be shown + /// + /// # Examples + /// + /// ```rust + /// # use clap::Command; + /// # use clap::Arg; + /// let cmd = Command::new("mycmd") + /// .arg( + /// Arg::new("special-help") + /// .short('?') + /// .action(clap::builder::ArgAction::Help) + /// ); + /// + /// // Existing help still exists + /// let err = cmd.clone().try_get_matches_from(["mycmd", "-h"]).unwrap_err(); + /// assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp); + /// + /// // New help available + /// let err = cmd.try_get_matches_from(["mycmd", "-?"]).unwrap_err(); + /// assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp); + /// ``` Help, + /// When encountered, display [`Command::version`][super::App::version] + /// + /// Depending on the flag, [`Command::long_version`][super::App::long_version] may be shown + /// + /// # Examples + /// + /// ```rust + /// # use clap::Command; + /// # use clap::Arg; + /// let cmd = Command::new("mycmd") + /// .version("1.0.0") + /// .arg( + /// Arg::new("special-version") + /// .long("special-version") + /// .action(clap::builder::ArgAction::Version) + /// ); + /// + /// // Existing help still exists + /// let err = cmd.clone().try_get_matches_from(["mycmd", "--version"]).unwrap_err(); + /// assert_eq!(err.kind(), clap::error::ErrorKind::DisplayVersion); + /// + /// // New help available + /// let err = cmd.try_get_matches_from(["mycmd", "--special-version"]).unwrap_err(); + /// assert_eq!(err.kind(), clap::error::ErrorKind::DisplayVersion); + /// ``` Version, } diff --git a/src/builder/arg.rs b/src/builder/arg.rs index 3b2469f894e..fa4e870ab95 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -1003,6 +1003,36 @@ impl<'help> Arg<'help> { } } + /// Specify the behavior when parsing an argument + /// + /// # Examples + /// + /// ```rust + /// # use clap::Command; + /// # use clap::Arg; + /// let cmd = Command::new("mycmd") + /// .arg( + /// Arg::new("flag") + /// .long("flag") + /// .takes_value(true) + /// .action(clap::builder::ArgAction::StoreValue) + /// ); + /// + /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "value"]).unwrap(); + /// assert!(matches.is_present("flag")); + /// assert_eq!(matches.occurrences_of("flag"), 1); + /// assert_eq!( + /// matches.get_many::("flag").unwrap_or_default().map(|v| v.as_str()).collect::>(), + /// vec!["value"] + /// ); + /// ``` + #[inline] + #[must_use] + pub fn action(mut self, action: ArgAction) -> Self { + self.action = Some(action); + self + } + /// Specify the type of the argument. /// /// This allows parsing and validating a value before storing it into @@ -4531,7 +4561,7 @@ impl<'help> Arg<'help> { } /// Behavior when parsing the argument - pub(crate) fn get_action(&self) -> &super::ArgAction { + pub fn get_action(&self) -> &super::ArgAction { const DEFAULT: super::ArgAction = super::ArgAction::StoreValue; self.action.as_ref().unwrap_or(&DEFAULT) } diff --git a/src/builder/mod.rs b/src/builder/mod.rs index ad74e4020d8..3c59b37350a 100644 --- a/src/builder/mod.rs +++ b/src/builder/mod.rs @@ -24,6 +24,7 @@ mod debug_asserts; #[cfg(test)] mod tests; +pub use action::ArgAction; pub use app_settings::{AppFlags, AppSettings}; pub use arg::Arg; pub use arg_group::ArgGroup; @@ -54,7 +55,6 @@ pub use command::App; #[cfg(feature = "regex")] pub use self::regex::RegexRef; -pub(crate) use action::ArgAction; pub(crate) use arg::display_arg_val; pub(crate) use arg_predicate::ArgPredicate; pub(crate) use value_parser::ValueParserInner; From e53dd937bee8bc96f617ded67b01355d288727b9 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 16:04:02 -0500 Subject: [PATCH 7/9] feat(builder): Infer takes_vaue from action --- src/builder/action.rs | 1 - src/builder/arg.rs | 8 +++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/builder/action.rs b/src/builder/action.rs index f83e62d151b..0f58738dbfd 100644 --- a/src/builder/action.rs +++ b/src/builder/action.rs @@ -35,7 +35,6 @@ pub enum ArgAction { /// .arg( /// Arg::new("flag") /// .long("flag") - /// .takes_value(true) /// .action(clap::builder::ArgAction::StoreValue) /// ); /// diff --git a/src/builder/arg.rs b/src/builder/arg.rs index fa4e870ab95..d8892cef9d6 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -1014,7 +1014,6 @@ impl<'help> Arg<'help> { /// .arg( /// Arg::new("flag") /// .long("flag") - /// .takes_value(true) /// .action(clap::builder::ArgAction::StoreValue) /// ); /// @@ -4892,6 +4891,13 @@ impl<'help> Arg<'help> { if self.is_positional() { self.settings.set(ArgSettings::TakesValue); } + if let Some(action) = self.action.as_ref() { + if action.takes_value() { + self.settings.set(ArgSettings::TakesValue); + } else { + self.settings.unset(ArgSettings::TakesValue); + } + } if self.value_parser.is_none() { if self.is_allow_invalid_utf8_set() { From 7163f8d3e823e06d728812cb11e746aac0801130 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 16:31:11 -0500 Subject: [PATCH 8/9] refactor(parser): Clarify intent of split_arg_values --- src/parser/parser.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 2c958c2c2fe..4f5a5fca524 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -347,7 +347,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // get the option so we can check the settings let arg_values = matcher.pending_values_mut(id, None); let arg = &self.cmd[id]; - let parse_result = self.push_arg_values( + let parse_result = self.split_arg_values( arg, arg_os.to_value_os(), trailing_values, @@ -389,7 +389,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } let arg_values = matcher.pending_values_mut(&arg.id, Some(Identifier::Index)); let _parse_result = - self.push_arg_values(arg, arg_os.to_value_os(), trailing_values, arg_values); + self.split_arg_values(arg, arg_os.to_value_os(), trailing_values, arg_values); if let Some(_parse_result) = _parse_result { if _parse_result != ParseResult::ValuesDone { debug!( @@ -963,7 +963,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { 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.push_arg_values( + let _parse_result = self.split_arg_values( arg, &RawOsStr::new(v), trailing_values, @@ -997,7 +997,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } else if let Some(v) = attached_value { let mut arg_values = Vec::new(); - let parse_result = self.push_arg_values(arg, v, trailing_values, &mut arg_values); + let parse_result = self.split_arg_values(arg, v, trailing_values, &mut arg_values); let react_result = self.react( Some(ident), ValueSource::CommandLine, @@ -1026,16 +1026,16 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } - fn push_arg_values( + fn split_arg_values( &self, arg: &Arg<'help>, val: &RawOsStr, trailing_values: bool, output: &mut Vec, ) -> Option { - debug!("Parser::push_arg_values; arg={}, val={:?}", arg.name, val); + debug!("Parser::split_arg_values; arg={}, val={:?}", arg.name, val); debug!( - "Parser::push_arg_values; trailing_values={:?}, DontDelimTrailingVals={:?}", + "Parser::split_arg_values; trailing_values={:?}, DontDelimTrailingVals={:?}", trailing_values, self.cmd.is_dont_delimit_trailing_values_set() ); @@ -1254,7 +1254,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { ); let mut arg_values = Vec::new(); let _parse_result = - self.push_arg_values(arg, &val, trailing_values, &mut arg_values); + self.split_arg_values(arg, &val, trailing_values, &mut arg_values); let _ = self.react(None, ValueSource::EnvVariable, arg, arg_values, matcher)?; if let Some(_parse_result) = _parse_result { if _parse_result != ParseResult::ValuesDone { @@ -1324,7 +1324,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // 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.push_arg_values( + let _parse_result = self.split_arg_values( arg, &RawOsStr::new(v), trailing_values, @@ -1377,7 +1377,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { if add { if let Some(default) = default { let mut arg_values = Vec::new(); - let _parse_result = self.push_arg_values( + let _parse_result = self.split_arg_values( arg, &RawOsStr::new(default), trailing_values, @@ -1416,7 +1416,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name); let mut arg_values = Vec::new(); for v in arg.default_vals.iter() { - let _parse_result = self.push_arg_values( + let _parse_result = self.split_arg_values( arg, &RawOsStr::new(v), trailing_values, From 36680593bec619ad5c506166defabfd65f307d18 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 31 May 2022 16:31:31 -0500 Subject: [PATCH 9/9] refactor(parser): Clarify intent of push_arg_values --- src/parser/parser.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 4f5a5fca524..61b17eacf13 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -1070,13 +1070,13 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } - fn store_arg_values( + fn push_arg_values( &self, arg: &Arg<'help>, raw_vals: Vec, matcher: &mut ArgMatcher, ) -> ClapResult<()> { - debug!("Parser::store_arg_values: {:?}", raw_vals); + debug!("Parser::push_arg_values: {:?}", raw_vals); for raw_val in raw_vals { // update the current index because each value is a distinct index to clap @@ -1154,7 +1154,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } else { self.start_custom_arg(matcher, arg, source); } - self.store_arg_values(arg, raw_vals, matcher)?; + self.push_arg_values(arg, raw_vals, matcher)?; if ident == Some(Identifier::Index) && arg.is_multiple_values_set() { // HACK: Maintain existing occurrence behavior let matched = matcher.get_mut(&arg.id).unwrap(); @@ -1337,7 +1337,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { } } self.start_custom_arg(matcher, arg, ValueSource::CommandLine); - self.store_arg_values(arg, arg_values, matcher)?; + self.push_arg_values(arg, arg_values, matcher)?; } None => { debug!("Parser::add_default_value:iter:{}: wasn't used", arg.name);