Skip to content

Commit

Permalink
fix(parser): Provide default value for Actions
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@epage
epage committed Jun 3, 2022
1 parent e8ad62b commit e4b443d
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 26 deletions.
50 changes: 47 additions & 3 deletions src/builder/action.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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::<bool>("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::<bool>("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
Expand All @@ -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::<bool>("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::<bool>("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
Expand All @@ -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::<u64>("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::<u64>("flag").copied(),
/// Some(0)
/// );
/// ```
Count,
/// When encountered, display [`Command::print_help`][super::App::print_help]
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 5 additions & 0 deletions src/builder/arg.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
70 changes: 47 additions & 23 deletions tests/builder/action.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::<bool>("mammal"), None);
assert_eq!(matches.is_present("mammal"), false);
assert_eq!(*matches.get_one::<bool>("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()
Expand All @@ -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")
Expand Down Expand Up @@ -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::<bool>("dog").unwrap(), false);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), false);

let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap();
assert_eq!(*matches.get_one::<bool>("dog").unwrap(), true);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), true);
Expand All @@ -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::<bool>("dog"), None);
assert_eq!(*matches.get_one::<bool>("dog").unwrap(), false);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), true);
}

Expand All @@ -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::<bool>("dog").unwrap(), false);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), false);

let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap();
assert_eq!(*matches.get_one::<bool>("dog").unwrap(), true);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), true);
Expand All @@ -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::<bool>("dog"), None);
assert_eq!(*matches.get_one::<bool>("dog").unwrap(), false);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), true);
}

Expand All @@ -191,12 +199,12 @@ fn set_true_with_required_if_eq() {
.clone()
.try_get_matches_from(["test", "--mammal"])
.unwrap();
assert_eq!(matches.get_one::<u64>("dog"), None);
assert_eq!(*matches.get_one::<bool>("dog").unwrap(), false);
assert_eq!(*matches.get_one::<bool>("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::<bool>("dog").unwrap(), true);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), false);

let matches = cmd
.clone()
Expand All @@ -215,10 +223,10 @@ fn set_false() {
);

let matches = cmd.clone().try_get_matches_from(["test"]).unwrap();
assert_eq!(matches.get_one::<bool>("mammal"), None);
assert_eq!(matches.is_present("mammal"), false);
assert_eq!(*matches.get_one::<bool>("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()
Expand All @@ -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")
Expand Down Expand Up @@ -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::<bool>("dog").unwrap(), true);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), true);

let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap();
assert_eq!(*matches.get_one::<bool>("dog").unwrap(), false);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), false);
Expand All @@ -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::<bool>("dog"), None);
assert_eq!(*matches.get_one::<bool>("dog").unwrap(), true);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), false);
}

Expand All @@ -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::<bool>("dog").unwrap(), true);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), true);

let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap();
assert_eq!(*matches.get_one::<bool>("dog").unwrap(), false);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), false);
Expand All @@ -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::<bool>("dog"), None);
assert_eq!(*matches.get_one::<bool>("dog").unwrap(), true);
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), false);
}

Expand All @@ -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::<u64>("mammal"), None);
assert_eq!(matches.is_present("mammal"), false);
assert_eq!(*matches.get_one::<u64>("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()
Expand All @@ -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")
Expand Down Expand Up @@ -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::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 0);

let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 1);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 10);
Expand All @@ -383,7 +403,7 @@ fn count_with_default_value_if_present() {
.clone()
.try_get_matches_from(["test", "--mammal"])
.unwrap();
assert_eq!(matches.get_one::<u64>("dog"), None);
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 1);
}

Expand All @@ -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::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 0);

let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 1);
assert_eq!(matches.get_one::<u64>("mammal"), None);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 0);

let matches = cmd
.clone()
Expand All @@ -413,6 +437,6 @@ fn count_with_default_value_if_value() {
.clone()
.try_get_matches_from(["test", "--mammal"])
.unwrap();
assert_eq!(matches.get_one::<u64>("dog"), None);
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 1);
}

0 comments on commit e4b443d

Please sign in to comment.