Skip to content

Commit

Permalink
fix!: Make ArgAction::Set the default
Browse files Browse the repository at this point in the history 
This removes the need for `TakesValue` bookkeeping for when we knew we
took values but didn't know how many we should take.

Fixes clap-rs#2687
  • Loading branch information
@epage
epage committed Aug 5, 2022
1 parent 8ed35b4 commit c801e4e
Show file tree
Hide file tree
Showing 10 changed files with 74 additions and 51 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Expand Up @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- `arg!` now sets `ArgAction::SetTrue`, `ArgAction::Count`, `ArgAction::Set`, or `ArgAction::Append` as appropriate (#3795)
- Default actions are now `Set` and `SetTrue`
- Removed `PartialEq` and `Eq` from `Command`
- By default, an `Arg`s default action is `ArgAction::Set`, rather than `ArgAction::SetTrue`
- Replace `Arg::number_of_values` (average across occurrences) with `Arg::num_args` (per occurrence, raw CLI args, not parsed values)
- `num_args(0)` no longer implies `takes_value(true).multiple_values(true)`
- `num_args(1)` no longer implies `multiple_values(true)`
Expand Down
3 changes: 2 additions & 1 deletion clap_bench/benches/03_complex.rs
View file
Expand Up @@ -67,12 +67,13 @@ pub fn build_from_builder(c: &mut Criterion) {
.help("tests flags")
.long("flag")
.global(true)
.action(ArgAction::Append),
.action(ArgAction::Count),
)
.arg(
Arg::new("flag2")
.short('F')
.help("tests flags with exclusions")
.action(ArgAction::SetTrue)
.conflicts_with("flag")
.requires("option2"),
)
Expand Down
4 changes: 3 additions & 1 deletion clap_bench/benches/04_new_help.rs
View file
Expand Up @@ -42,7 +42,8 @@ fn app_example3<'c>() -> Command<'c> {
.arg(
Arg::new("debug")
.help("turn on debugging information")
.short('d'),
.short('d')
.action(ArgAction::SetTrue),
)
.args(&[
Arg::new("config")
Expand Down Expand Up @@ -73,6 +74,7 @@ fn app_example4<'c>() -> Command<'c> {
Arg::new("debug")
.help("turn on debugging information")
.short('d')
.action(ArgAction::SetTrue)
.long("debug"),
)
.arg(
Expand Down
15 changes: 13 additions & 2 deletions clap_bench/benches/05_ripgrep.rs
View file
Expand Up @@ -299,7 +299,7 @@ where
F: Fn(&'static str) -> &'static str,
{
let arg = |name| Arg::new(name).help(doc(name));
let flag = |name| arg(name).long(name);
let flag = |name| arg(name).long(name).action(ArgAction::SetTrue);

Command::new("ripgrep")
.author("BurntSushi") // simulating since it's only a bench
Expand Down Expand Up @@ -379,18 +379,21 @@ where
.arg(
flag("after-context")
.short('A')
.action(ArgAction::Set)
.value_name("NUM")
.value_parser(value_parser!(usize)),
)
.arg(
flag("before-context")
.short('B')
.action(ArgAction::Set)
.value_name("NUM")
.value_parser(value_parser!(usize)),
)
.arg(
flag("context")
.short('C')
.action(ArgAction::Set)
.value_name("NUM")
.value_parser(value_parser!(usize)),
)
Expand Down Expand Up @@ -419,11 +422,13 @@ where
.arg(
flag("max-count")
.short('m')
.action(ArgAction::Set)
.value_name("NUM")
.value_parser(value_parser!(usize)),
)
.arg(
flag("maxdepth")
.action(ArgAction::Set)
.value_name("NUM")
.value_parser(value_parser!(usize)),
)
Expand All @@ -436,13 +441,19 @@ where
.arg(flag("null"))
.arg(flag("path-separator").value_name("SEPARATOR"))
.arg(flag("pretty").short('p'))
.arg(flag("replace").short('r').value_name("ARG"))
.arg(
flag("replace")
.short('r')
.action(ArgAction::Set)
.value_name("ARG"),
)
.arg(flag("case-sensitive").short('s'))
.arg(flag("smart-case").short('S'))
.arg(flag("sort-files"))
.arg(
flag("threads")
.short('j')
.action(ArgAction::Set)
.value_name("ARG")
.value_parser(value_parser!(usize)),
)
Expand Down
10 changes: 8 additions & 2 deletions clap_bench/benches/06_rustup.rs
View file
Expand Up @@ -30,7 +30,8 @@ fn build_cli() -> Command<'static> {
Arg::new("verbose")
.help("Enable verbose output")
.short('v')
.long("verbose"),
.long("verbose")
.action(ArgAction::SetTrue),
)
.subcommand(
Command::new("show")
Expand All @@ -53,6 +54,7 @@ fn build_cli() -> Command<'static> {
Arg::new("no-self-update")
.help("Don't perform self update when running the `rustup` command")
.long("no-self-update")
.action(ArgAction::SetTrue)
.hide(true),
),
)
Expand Down Expand Up @@ -210,6 +212,7 @@ fn build_cli() -> Command<'static> {
.arg(
Arg::new("nonexistent")
.long("nonexistent")
.action(ArgAction::SetTrue)
.help("Remove override toolchain for all nonexistent directories"),
),
)
Expand All @@ -226,6 +229,7 @@ fn build_cli() -> Command<'static> {
.arg(
Arg::new("nonexistent")
.long("nonexistent")
.action(ArgAction::SetTrue)
.help("Remove override toolchain for all nonexistent directories"),
),
),
Expand All @@ -250,11 +254,13 @@ fn build_cli() -> Command<'static> {
.arg(
Arg::new("book")
.long("book")
.action(ArgAction::SetTrue)
.help("The Rust Programming Language book"),
)
.arg(
Arg::new("std")
.long("std")
.action(ArgAction::SetTrue)
.help("Standard library API documentation"),
)
.group(ArgGroup::new("page").args(&["book", "std"])),
Expand All @@ -276,7 +282,7 @@ fn build_cli() -> Command<'static> {
.subcommand(
Command::new("uninstall")
.about("Uninstall rustup.")
.arg(Arg::new("no-prompt").short('y')),
.arg(Arg::new("no-prompt").short('y').action(ArgAction::SetTrue)),
)
.subcommand(
Command::new("upgrade-data").about("Upgrade the internal data format."),
Expand Down
75 changes: 34 additions & 41 deletions src/builder/arg.rs
View file
Expand Up @@ -778,16 +778,6 @@ impl<'help> Arg<'help> {

/// # Value Handling
impl<'help> Arg<'help> {
#[inline]
#[must_use]
fn takes_value(self, yes: bool) -> Self {
if yes {
self.setting(ArgSettings::TakesValue)
} else {
self.unset_setting(ArgSettings::TakesValue)
}
}

/// Specify the behavior when parsing an argument
///
/// # Examples
Expand Down Expand Up @@ -1024,7 +1014,7 @@ impl<'help> Arg<'help> {
pub fn num_args(mut self, qty: impl Into<ValueRange>) -> Self {
let qty = qty.into();
self.num_vals = Some(qty);
self.takes_value(qty.takes_values())
self
}

/// Placeholder for the argument's value in the help message / usage.
Expand Down Expand Up @@ -1134,7 +1124,7 @@ impl<'help> Arg<'help> {
#[must_use]
pub fn value_names(mut self, names: &[&'help str]) -> Self {
self.val_names = names.to_vec();
self.takes_value(true)
self
}

/// Provide the shell a hint about how to complete this argument.
Expand Down Expand Up @@ -1169,7 +1159,7 @@ impl<'help> Arg<'help> {
#[must_use]
pub fn value_hint(mut self, value_hint: ValueHint) -> Self {
self.value_hint = Some(value_hint);
self.takes_value(true)
self
}

/// Match values against [`PossibleValuesParser`][crate::builder::PossibleValuesParser] without matching case.
Expand Down Expand Up @@ -1370,7 +1360,7 @@ impl<'help> Arg<'help> {
#[must_use]
pub fn value_delimiter(mut self, d: impl Into<Option<char>>) -> Self {
self.val_delim = d.into();
self.takes_value(true)
self
}

/// Sentinel to **stop** parsing multiple values of a given argument.
Expand Down Expand Up @@ -1423,7 +1413,7 @@ impl<'help> Arg<'help> {
#[must_use]
pub fn value_terminator(mut self, term: &'help str) -> Self {
self.terminator = Some(term);
self.takes_value(true)
self
}

/// Consume all following arguments.
Expand Down Expand Up @@ -1451,8 +1441,10 @@ impl<'help> Arg<'help> {
#[inline]
#[must_use]
pub fn raw(mut self, yes: bool) -> Self {
self.num_vals.get_or_insert_with(|| (1..).into());
self.takes_value(yes).allow_hyphen_values(yes).last(yes)
if yes {
self.num_vals.get_or_insert_with(|| (1..).into());
}
self.allow_hyphen_values(yes).last(yes)
}

/// Value for the argument when not present.
Expand Down Expand Up @@ -1549,7 +1541,7 @@ impl<'help> Arg<'help> {
#[must_use]
pub fn default_values_os(mut self, vals: &[&'help OsStr]) -> Self {
self.default_vals = vals.to_vec();
self.takes_value(true)
self
}

/// Value for the argument when the flag is present but no value is specified.
Expand Down Expand Up @@ -1681,7 +1673,7 @@ impl<'help> Arg<'help> {
#[must_use]
pub fn default_missing_values_os(mut self, vals: &[&'help OsStr]) -> Self {
self.default_missing_vals = vals.to_vec();
self.takes_value(true)
self
}

/// Read from `name` environment variable when argument is not present.
Expand Down Expand Up @@ -1733,7 +1725,7 @@ impl<'help> Arg<'help> {
///
/// ```rust
/// # use std::env;
/// # use clap::{Command, Arg};
/// # use clap::{Command, Arg, ArgAction};
/// # use clap::builder::FalseyValueParser;
///
/// env::set_var("TRUE_FLAG", "true");
Expand All @@ -1742,14 +1734,17 @@ impl<'help> Arg<'help> {
/// let m = Command::new("prog")
/// .arg(Arg::new("true_flag")
/// .long("true_flag")
/// .action(ArgAction::SetTrue)
/// .value_parser(FalseyValueParser::new())
/// .env("TRUE_FLAG"))
/// .arg(Arg::new("false_flag")
/// .long("false_flag")
/// .action(ArgAction::SetTrue)
/// .value_parser(FalseyValueParser::new())
/// .env("FALSE_FLAG"))
/// .arg(Arg::new("absent_flag")
/// .long("absent_flag")
/// .action(ArgAction::SetTrue)
/// .value_parser(FalseyValueParser::new())
/// .env("ABSENT_FLAG"))
/// .get_matches_from(vec![
Expand Down Expand Up @@ -2621,7 +2616,7 @@ impl<'help> Arg<'help> {
) -> Self {
self.default_vals_ifs
.push((arg_id.into(), val.into(), default));
self.takes_value(true)
self
}

/// Specifies multiple values and conditions in the same manner as [`Arg::default_value_if`].
Expand Down Expand Up @@ -3401,7 +3396,8 @@ impl<'help> Arg<'help> {
/// .conflicts_with("debug")
/// .long("config"))
/// .arg(Arg::new("debug")
/// .long("debug"))
/// .long("debug")
/// .action(ArgAction::SetTrue))
/// .try_get_matches_from(vec![
/// "prog", "--debug", "--config", "file.conf"
/// ]);
Expand Down Expand Up @@ -3782,7 +3778,7 @@ impl<'help> Arg<'help> {
}

pub(crate) fn is_takes_value_set(&self) -> bool {
self.is_set(ArgSettings::TakesValue)
self.get_action().takes_values()
}

/// Report whether [`Arg::allow_hyphen_values`] is set
Expand Down Expand Up @@ -3891,27 +3887,28 @@ impl<'help> Arg<'help> {
/// # Internally used only
impl<'help> Arg<'help> {
pub(crate) fn _build(&mut self) {
if self.is_positional() {
self.settings.set(ArgSettings::TakesValue);
}
if self.action.is_none() {
if self.get_id() == "help" && !self.is_takes_value_set() {
if self.get_id() == "help"
&& matches!(self.get_num_args(), Some(ValueRange::EMPTY) | None)
{
let action = super::ArgAction::Help;
self.action = Some(action);
} else if self.get_id() == "version" && !self.is_takes_value_set() {
} else if self.get_id() == "version"
&& matches!(self.get_num_args(), Some(ValueRange::EMPTY) | None)
{
let action = super::ArgAction::Version;
self.action = Some(action);
} else if self.is_takes_value_set() {
} else if self.num_vals == Some(ValueRange::EMPTY) {
let action = super::ArgAction::SetTrue;
self.action = Some(action);
} else {
let action = if self.is_positional() && self.is_multiple_values_set() {
// Allow collecting arguments interleaved with flags
super::ArgAction::Append
} else {
super::ArgAction::Set
};
self.action = Some(action);
} else {
let action = super::ArgAction::SetTrue;
self.action = Some(action);
}
}
if let Some(action) = self.action.as_ref() {
Expand All @@ -3925,11 +3922,6 @@ impl<'help> Arg<'help> {
self.default_missing_vals = vec![default_value];
}
}
if action.takes_values() {
self.settings.set(ArgSettings::TakesValue);
} else {
self.settings.unset(ArgSettings::TakesValue);
}
}

if self.value_parser.is_none() {
Expand All @@ -3944,11 +3936,12 @@ impl<'help> Arg<'help> {
if val_names_len > 1 {
self.num_vals.get_or_insert(val_names_len.into());
} else {
if self.is_takes_value_set() {
self.num_vals.get_or_insert(ValueRange::SINGLE);
let nargs = if self.get_action().takes_values() {
ValueRange::SINGLE
} else {
self.num_vals.get_or_insert(ValueRange::EMPTY);
}
ValueRange::EMPTY
};
self.num_vals.get_or_insert(nargs);
}
}

Expand Down

0 comments on commit c801e4e

Please sign in to comment.