From e4b443d8bbd38970f3790fa31964c30b2710d080 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 3 Jun 2022 10:01:49 -0500 Subject: [PATCH] fix(parser): Provide default value for Actions Actions were inspired by Python and Python does not implicitly default any field when an action is given. From a Builder API perspective, this seemed fine because we tend to focus the Builder API on giving the user all information so they can make their own decisions. When working on the Derive API, this became a problem because users were going to have to migrate from an implied default to an explicit default when a common default is good enough most of the time. This shouldn't interfere with Builder users getting more details when needed. This also highlighted two problems - We set the index for defaults - We don't debug_assert when applying conditional requirements with a default present --- src/builder/action.rs | 50 +++++++++++++++++++++++++++-- src/builder/arg.rs | 5 +++ tests/builder/action.rs | 70 +++++++++++++++++++++++++++-------------- 3 files changed, 99 insertions(+), 26 deletions(-) diff --git a/src/builder/action.rs b/src/builder/action.rs index ff5890a51ea..fa1ccd57eef 100644 --- a/src/builder/action.rs +++ b/src/builder/action.rs @@ -119,6 +119,8 @@ pub enum ArgAction { IncOccurrence, /// When encountered, act as if `"true"` was encountered on the command-line /// + /// If no [`default_value`][super::Arg::default_value] is set, it will be `false`. + /// /// No value is allowed /// /// # Examples @@ -133,17 +135,27 @@ pub enum ArgAction { /// .action(clap::builder::ArgAction::SetTrue) /// ); /// - /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); + /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); /// assert!(matches.is_present("flag")); /// assert_eq!(matches.occurrences_of("flag"), 0); /// assert_eq!( /// matches.get_one::("flag").copied(), /// Some(true) /// ); + /// + /// let matches = cmd.try_get_matches_from(["mycmd"]).unwrap(); + /// assert!(matches.is_present("flag")); + /// assert_eq!(matches.occurrences_of("flag"), 0); + /// assert_eq!( + /// matches.get_one::("flag").copied(), + /// Some(false) + /// ); /// ``` SetTrue, /// When encountered, act as if `"false"` was encountered on the command-line /// + /// If no [`default_value`][super::Arg::default_value] is set, it will be `true`. + /// /// No value is allowed /// /// # Examples @@ -158,17 +170,27 @@ pub enum ArgAction { /// .action(clap::builder::ArgAction::SetFalse) /// ); /// - /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); + /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); /// assert!(matches.is_present("flag")); /// assert_eq!(matches.occurrences_of("flag"), 0); /// assert_eq!( /// matches.get_one::("flag").copied(), /// Some(false) /// ); + /// + /// let matches = cmd.try_get_matches_from(["mycmd"]).unwrap(); + /// assert!(matches.is_present("flag")); + /// assert_eq!(matches.occurrences_of("flag"), 0); + /// assert_eq!( + /// matches.get_one::("flag").copied(), + /// Some(true) + /// ); /// ``` SetFalse, /// When encountered, increment a `u64` counter /// + /// If no [`default_value`][super::Arg::default_value] is set, it will be `0`. + /// /// No value is allowed /// /// # Examples @@ -183,13 +205,21 @@ pub enum ArgAction { /// .action(clap::builder::ArgAction::Count) /// ); /// - /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); + /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); /// assert!(matches.is_present("flag")); /// assert_eq!(matches.occurrences_of("flag"), 0); /// assert_eq!( /// matches.get_one::("flag").copied(), /// Some(2) /// ); + /// + /// let matches = cmd.try_get_matches_from(["mycmd"]).unwrap(); + /// assert!(matches.is_present("flag")); + /// assert_eq!(matches.occurrences_of("flag"), 0); + /// assert_eq!( + /// matches.get_one::("flag").copied(), + /// Some(0) + /// ); /// ``` Count, /// When encountered, display [`Command::print_help`][super::App::print_help] @@ -246,6 +276,20 @@ pub enum ArgAction { } impl ArgAction { + pub(crate) fn default_value(&self) -> Option<&'static std::ffi::OsStr> { + match self { + Self::Set => None, + Self::Append => None, + Self::StoreValue => None, + Self::IncOccurrence => None, + Self::SetTrue => Some(std::ffi::OsStr::new("false")), + Self::SetFalse => Some(std::ffi::OsStr::new("true")), + Self::Count => Some(std::ffi::OsStr::new("0")), + Self::Help => None, + Self::Version => None, + } + } + pub(crate) fn takes_value(&self) -> bool { match self { Self::Set => true, diff --git a/src/builder/arg.rs b/src/builder/arg.rs index 9f62e06241c..6f9fc767887 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -4892,6 +4892,11 @@ impl<'help> Arg<'help> { self.settings.set(ArgSettings::TakesValue); } if let Some(action) = self.action.as_ref() { + if let Some(default_value) = action.default_value() { + if self.default_vals.is_empty() { + self.default_vals = vec![default_value]; + } + } if action.takes_value() { self.settings.set(ArgSettings::TakesValue); } else { diff --git a/tests/builder/action.rs b/tests/builder/action.rs index 25abb1de4f1..1697914d84a 100644 --- a/tests/builder/action.rs +++ b/tests/builder/action.rs @@ -81,10 +81,10 @@ fn set_true() { Command::new("test").arg(Arg::new("mammal").long("mammal").action(ArgAction::SetTrue)); let matches = cmd.clone().try_get_matches_from(["test"]).unwrap(); - assert_eq!(matches.get_one::("mammal"), None); - assert_eq!(matches.is_present("mammal"), false); + assert_eq!(*matches.get_one::("mammal").unwrap(), false); + assert_eq!(matches.is_present("mammal"), true); assert_eq!(matches.occurrences_of("mammal"), 0); - assert_eq!(matches.index_of("mammal"), None); + assert_eq!(matches.index_of("mammal"), Some(1)); let matches = cmd .clone() @@ -106,7 +106,7 @@ fn set_true() { } #[test] -fn set_true_with_default_value() { +fn set_true_with_explicit_default_value() { let cmd = Command::new("test").arg( Arg::new("mammal") .long("mammal") @@ -141,6 +141,10 @@ fn set_true_with_default_value_if_present() { ) .arg(Arg::new("dog").long("dog").action(ArgAction::SetTrue)); + let matches = cmd.clone().try_get_matches_from(["test"]).unwrap(); + assert_eq!(*matches.get_one::("dog").unwrap(), false); + assert_eq!(*matches.get_one::("mammal").unwrap(), false); + let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap(); assert_eq!(*matches.get_one::("dog").unwrap(), true); assert_eq!(*matches.get_one::("mammal").unwrap(), true); @@ -149,7 +153,7 @@ fn set_true_with_default_value_if_present() { .clone() .try_get_matches_from(["test", "--mammal"]) .unwrap(); - assert_eq!(matches.get_one::("dog"), None); + assert_eq!(*matches.get_one::("dog").unwrap(), false); assert_eq!(*matches.get_one::("mammal").unwrap(), true); } @@ -164,6 +168,10 @@ fn set_true_with_default_value_if_value() { ) .arg(Arg::new("dog").long("dog").action(ArgAction::SetTrue)); + let matches = cmd.clone().try_get_matches_from(["test"]).unwrap(); + assert_eq!(*matches.get_one::("dog").unwrap(), false); + assert_eq!(*matches.get_one::("mammal").unwrap(), false); + let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap(); assert_eq!(*matches.get_one::("dog").unwrap(), true); assert_eq!(*matches.get_one::("mammal").unwrap(), true); @@ -172,7 +180,7 @@ fn set_true_with_default_value_if_value() { .clone() .try_get_matches_from(["test", "--mammal"]) .unwrap(); - assert_eq!(matches.get_one::("dog"), None); + assert_eq!(*matches.get_one::("dog").unwrap(), false); assert_eq!(*matches.get_one::("mammal").unwrap(), true); } @@ -191,12 +199,12 @@ fn set_true_with_required_if_eq() { .clone() .try_get_matches_from(["test", "--mammal"]) .unwrap(); - assert_eq!(matches.get_one::("dog"), None); + assert_eq!(*matches.get_one::("dog").unwrap(), false); assert_eq!(*matches.get_one::("mammal").unwrap(), true); - cmd.clone() - .try_get_matches_from(["test", "--dog"]) - .unwrap_err(); + let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap(); + assert_eq!(*matches.get_one::("dog").unwrap(), true); + assert_eq!(*matches.get_one::("mammal").unwrap(), false); let matches = cmd .clone() @@ -215,10 +223,10 @@ fn set_false() { ); let matches = cmd.clone().try_get_matches_from(["test"]).unwrap(); - assert_eq!(matches.get_one::("mammal"), None); - assert_eq!(matches.is_present("mammal"), false); + assert_eq!(*matches.get_one::("mammal").unwrap(), true); + assert_eq!(matches.is_present("mammal"), true); assert_eq!(matches.occurrences_of("mammal"), 0); - assert_eq!(matches.index_of("mammal"), None); + assert_eq!(matches.index_of("mammal"), Some(1)); let matches = cmd .clone() @@ -240,7 +248,7 @@ fn set_false() { } #[test] -fn set_false_with_default_value() { +fn set_false_with_explicit_default_value() { let cmd = Command::new("test").arg( Arg::new("mammal") .long("mammal") @@ -275,6 +283,10 @@ fn set_false_with_default_value_if_present() { ) .arg(Arg::new("dog").long("dog").action(ArgAction::SetFalse)); + let matches = cmd.clone().try_get_matches_from(["test"]).unwrap(); + assert_eq!(*matches.get_one::("dog").unwrap(), true); + assert_eq!(*matches.get_one::("mammal").unwrap(), true); + let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap(); assert_eq!(*matches.get_one::("dog").unwrap(), false); assert_eq!(*matches.get_one::("mammal").unwrap(), false); @@ -283,7 +295,7 @@ fn set_false_with_default_value_if_present() { .clone() .try_get_matches_from(["test", "--mammal"]) .unwrap(); - assert_eq!(matches.get_one::("dog"), None); + assert_eq!(*matches.get_one::("dog").unwrap(), true); assert_eq!(*matches.get_one::("mammal").unwrap(), false); } @@ -298,6 +310,10 @@ fn set_false_with_default_value_if_value() { ) .arg(Arg::new("dog").long("dog").action(ArgAction::SetFalse)); + let matches = cmd.clone().try_get_matches_from(["test"]).unwrap(); + assert_eq!(*matches.get_one::("dog").unwrap(), true); + assert_eq!(*matches.get_one::("mammal").unwrap(), true); + let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap(); assert_eq!(*matches.get_one::("dog").unwrap(), false); assert_eq!(*matches.get_one::("mammal").unwrap(), false); @@ -306,7 +322,7 @@ fn set_false_with_default_value_if_value() { .clone() .try_get_matches_from(["test", "--mammal"]) .unwrap(); - assert_eq!(matches.get_one::("dog"), None); + assert_eq!(*matches.get_one::("dog").unwrap(), true); assert_eq!(*matches.get_one::("mammal").unwrap(), false); } @@ -315,10 +331,10 @@ fn count() { let cmd = Command::new("test").arg(Arg::new("mammal").long("mammal").action(ArgAction::Count)); let matches = cmd.clone().try_get_matches_from(["test"]).unwrap(); - assert_eq!(matches.get_one::("mammal"), None); - assert_eq!(matches.is_present("mammal"), false); + assert_eq!(*matches.get_one::("mammal").unwrap(), 0); + assert_eq!(matches.is_present("mammal"), true); assert_eq!(matches.occurrences_of("mammal"), 0); - assert_eq!(matches.index_of("mammal"), None); + assert_eq!(matches.index_of("mammal"), Some(1)); let matches = cmd .clone() @@ -340,7 +356,7 @@ fn count() { } #[test] -fn count_with_default_value() { +fn count_with_explicit_default_value() { let cmd = Command::new("test").arg( Arg::new("mammal") .long("mammal") @@ -375,6 +391,10 @@ fn count_with_default_value_if_present() { ) .arg(Arg::new("dog").long("dog").action(ArgAction::Count)); + let matches = cmd.clone().try_get_matches_from(["test"]).unwrap(); + assert_eq!(*matches.get_one::("dog").unwrap(), 0); + assert_eq!(*matches.get_one::("mammal").unwrap(), 0); + let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap(); assert_eq!(*matches.get_one::("dog").unwrap(), 1); assert_eq!(*matches.get_one::("mammal").unwrap(), 10); @@ -383,7 +403,7 @@ fn count_with_default_value_if_present() { .clone() .try_get_matches_from(["test", "--mammal"]) .unwrap(); - assert_eq!(matches.get_one::("dog"), None); + assert_eq!(*matches.get_one::("dog").unwrap(), 0); assert_eq!(*matches.get_one::("mammal").unwrap(), 1); } @@ -398,9 +418,13 @@ fn count_with_default_value_if_value() { ) .arg(Arg::new("dog").long("dog").action(ArgAction::Count)); + let matches = cmd.clone().try_get_matches_from(["test"]).unwrap(); + assert_eq!(*matches.get_one::("dog").unwrap(), 0); + assert_eq!(*matches.get_one::("mammal").unwrap(), 0); + let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap(); assert_eq!(*matches.get_one::("dog").unwrap(), 1); - assert_eq!(matches.get_one::("mammal"), None); + assert_eq!(*matches.get_one::("mammal").unwrap(), 0); let matches = cmd .clone() @@ -413,6 +437,6 @@ fn count_with_default_value_if_value() { .clone() .try_get_matches_from(["test", "--mammal"]) .unwrap(); - assert_eq!(matches.get_one::("dog"), None); + assert_eq!(*matches.get_one::("dog").unwrap(), 0); assert_eq!(*matches.get_one::("mammal").unwrap(), 1); }