Skip to content

Commit

Permalink
fix!: Change arg! to use ArgAction
Browse files Browse the repository at this point in the history 
Fixes clap-rs#3795
  • Loading branch information
@epage
epage committed Jul 22, 2022
1 parent 50019ca commit 04cdfb9
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 141 deletions.
22 changes: 9 additions & 13 deletions src/builder/arg.rs
View file
Expand Up @@ -3879,10 +3879,10 @@ impl<'help> Arg<'help> {
/// "prog", "-f", "-d", "-c"]);
/// // ^~~~~~~~~~~~^~~~~ flag is overridden by color
///
/// assert!(m.is_present("color"));
/// assert!(m.is_present("debug")); // even though flag conflicts with debug, it's as if flag
/// assert!(*m.get_one::<bool>("color").unwrap());
/// assert!(*m.get_one::<bool>("debug").unwrap()); // even though flag conflicts with debug, it's as if flag
/// // was never used because it was overridden with color
/// assert!(!m.is_present("flag"));
/// assert!(!*m.get_one::<bool>("flag").unwrap());
/// ```
/// Care must be taken when using this setting, and having an arg override with itself. This
/// is common practice when supporting things like shell aliases, config files, etc.
Expand All @@ -3897,19 +3897,18 @@ impl<'help> Arg<'help> {
/// let m = Command::new("posix")
/// .arg(arg!(--flag "some flag").overrides_with("flag"))
/// .get_matches_from(vec!["posix", "--flag", "--flag"]);
/// assert!(m.is_present("flag"));
/// assert!(*m.get_one::<bool>("flag").unwrap());
/// ```
///
/// Making an arg [`Arg::multiple_occurrences`] and override itself
/// is essentially meaningless. Therefore clap ignores an override of self
/// if it's a flag and it already accepts multiple occurrences.
/// is essentially meaningless. Therefore clap ignores an override of self.
///
/// ```
/// # use clap::{Command, arg};
/// let m = Command::new("posix")
/// .arg(arg!(--flag ... "some flag").overrides_with("flag"))
/// .get_matches_from(vec!["", "--flag", "--flag", "--flag", "--flag"]);
/// assert!(m.is_present("flag"));
/// assert_eq!(*m.get_one::<u8>("flag").unwrap(), 4);
/// ```
///
/// Now notice with options (which *do not* set
Expand All @@ -3921,7 +3920,6 @@ impl<'help> Arg<'help> {
/// let m = Command::new("posix")
/// .arg(arg!(--opt <val> "some option").overrides_with("opt"))
/// .get_matches_from(vec!["", "--opt=some", "--opt=other"]);
/// assert!(m.is_present("opt"));
/// assert_eq!(m.get_one::<String>("opt").unwrap(), "other");
/// ```
///
Expand All @@ -3938,7 +3936,6 @@ impl<'help> Arg<'help> {
/// .overrides_with("opt")
/// )
/// .get_matches_from(vec!["", "--opt", "1", "2", "--opt", "3", "4", "5"]);
/// assert!(m.is_present("opt"));
/// assert_eq!(m.get_many::<String>("opt").unwrap().collect::<Vec<_>>(), &["3", "4", "5"]);
/// ```
///
Expand All @@ -3952,7 +3949,6 @@ impl<'help> Arg<'help> {
/// .multiple_values(true)
/// .overrides_with("opt"))
/// .get_matches_from(vec!["", "--opt", "first", "over", "--opt", "other", "val"]);
/// assert!(m.is_present("opt"));
/// assert_eq!(m.get_many::<String>("opt").unwrap().collect::<Vec<_>>(), &["first", "over", "other", "val"]);
/// ```
#[must_use]
Expand Down Expand Up @@ -3985,11 +3981,11 @@ impl<'help> Arg<'help> {
/// "prog", "-f", "-d", "-c"]);
/// // ^~~~~~^~~~~~~~~ flag and debug are overridden by color
///
/// assert!(m.is_present("color")); // even though flag conflicts with color, it's as if flag
/// assert!(*m.get_one::<bool>("color").unwrap()); // even though flag conflicts with color, it's as if flag
/// // and debug were never used because they were overridden
/// // with color
/// assert!(!m.is_present("debug"));
/// assert!(!m.is_present("flag"));
/// assert!(!*m.get_one::<bool>("debug").unwrap());
/// assert!(!*m.get_one::<bool>("flag").unwrap());
/// ```
#[must_use]
pub fn overrides_with_all<T: Key>(mut self, names: &[T]) -> Self {
Expand Down
70 changes: 26 additions & 44 deletions src/macros.rs
View file
Expand Up @@ -200,17 +200,14 @@ macro_rules! arg_impl {
@arg
({
debug_assert_eq!($arg.get_value_names(), None, "Flags should precede values");
#[allow(deprecated)]
{
debug_assert!(!$arg.is_multiple_occurrences_set(), "Flags should precede `...`");
}
debug_assert!(!matches!($arg.get_action(), $crate::ArgAction::Append), "Flags should precede `...`");

let mut arg = $arg;
let long = $crate::arg_impl! { @string $long };
if arg.get_id().is_empty() {
arg = arg.id(long);
}
arg.long(long)
arg.long(long).action($crate::ArgAction::SetTrue)
})
$($tail)*
}
Expand All @@ -225,17 +222,14 @@ macro_rules! arg_impl {
@arg
({
debug_assert_eq!($arg.get_value_names(), None, "Flags should precede values");
#[allow(deprecated)]
{
debug_assert!(!$arg.is_multiple_occurrences_set(), "Flags should precede `...`");
}
debug_assert!(!matches!($arg.get_action(), $crate::ArgAction::Append), "Flags should precede `...`");

let mut arg = $arg;
let long = $crate::arg_impl! { @string $long };
if arg.get_id().is_empty() {
arg = arg.id(long);
}
arg.long(long)
arg.long(long).action($crate::ArgAction::SetTrue)
})
$($tail)*
}
Expand All @@ -251,12 +245,9 @@ macro_rules! arg_impl {
({
debug_assert_eq!($arg.get_long(), None, "Short flags should precede long flags");
debug_assert_eq!($arg.get_value_names(), None, "Flags should precede values");
#[allow(deprecated)]
{
debug_assert!(!$arg.is_multiple_occurrences_set(), "Flags should precede `...`");
}
debug_assert!(!matches!($arg.get_action(), $crate::ArgAction::Append), "Flags should precede `...`");

$arg.short($crate::arg_impl! { @char $short })
$arg.short($crate::arg_impl! { @char $short }).action($crate::ArgAction::SetTrue)
})
$($tail)*
}
Expand All @@ -272,12 +263,9 @@ macro_rules! arg_impl {
({
debug_assert_eq!($arg.get_long(), None, "Short flags should precede long flags");
debug_assert_eq!($arg.get_value_names(), None, "Flags should precede values");
#[allow(deprecated)]
{
debug_assert!(!$arg.is_multiple_occurrences_set(), "Flags should precede `...`");
}
debug_assert!(!matches!($arg.get_action(), $crate::ArgAction::Append), "Flags should precede `...`");

$arg.short($crate::arg_impl! { @char $short })
$arg.short($crate::arg_impl! { @char $short }).action($crate::ArgAction::SetTrue)
})
$($tail)*
}
Expand All @@ -291,10 +279,7 @@ macro_rules! arg_impl {
$crate::arg_impl! {
@arg
({
#[allow(deprecated)]
{
debug_assert!(!$arg.is_multiple_occurrences_set(), "Values should precede `...`");
}
debug_assert!(!matches!($arg.get_action(), $crate::ArgAction::Append), "Flags should precede `...`");
debug_assert_eq!($arg.get_value_names(), None, "Multiple values not yet supported");

let mut arg = $arg;
Expand All @@ -306,7 +291,7 @@ macro_rules! arg_impl {
if arg.get_id().is_empty() {
arg = arg.id(value_name);
}
arg.value_name(value_name)
arg.value_name(value_name).action($crate::ArgAction::Set)
})
$($tail)*
}
Expand All @@ -320,10 +305,7 @@ macro_rules! arg_impl {
$crate::arg_impl! {
@arg
({
#[allow(deprecated)]
{
debug_assert!(!$arg.is_multiple_occurrences_set(), "Values should precede `...`");
}
debug_assert!(!matches!($arg.get_action(), $crate::ArgAction::Append), "Flags should precede `...`");
debug_assert_eq!($arg.get_value_names(), None, "Multiple values not yet supported");

let mut arg = $arg;
Expand All @@ -335,7 +317,7 @@ macro_rules! arg_impl {
if arg.get_id().is_empty() {
arg = arg.id(value_name);
}
arg.value_name(value_name)
arg.value_name(value_name).action($crate::ArgAction::Set)
})
$($tail)*
}
Expand All @@ -349,10 +331,7 @@ macro_rules! arg_impl {
$crate::arg_impl! {
@arg
({
#[allow(deprecated)]
{
debug_assert!(!$arg.is_multiple_occurrences_set(), "Values should precede `...`");
}
debug_assert!(!matches!($arg.get_action(), $crate::ArgAction::Append), "Flags should precede `...`");
debug_assert_eq!($arg.get_value_names(), None, "Multiple values not yet supported");

let mut arg = $arg;
Expand All @@ -368,7 +347,7 @@ macro_rules! arg_impl {
if arg.get_id().is_empty() {
arg = arg.id(value_name);
}
arg.value_name(value_name)
arg.value_name(value_name).action($crate::ArgAction::Set)
})
$($tail)*
}
Expand All @@ -382,10 +361,7 @@ macro_rules! arg_impl {
$crate::arg_impl! {
@arg
({
#[allow(deprecated)]
{
debug_assert!(!$arg.is_multiple_occurrences_set(), "Values should precede `...`");
}
debug_assert!(!matches!($arg.get_action(), $crate::ArgAction::Append), "Flags should precede `...`");
debug_assert_eq!($arg.get_value_names(), None, "Multiple values not yet supported");

let mut arg = $arg;
Expand All @@ -401,7 +377,7 @@ macro_rules! arg_impl {
if arg.get_id().is_empty() {
arg = arg.id(value_name);
}
arg.value_name(value_name)
arg.value_name(value_name).action($crate::ArgAction::Set)
})
$($tail)*
}
Expand All @@ -414,9 +390,15 @@ macro_rules! arg_impl {
) => {
$crate::arg_impl! {
@arg
({#[allow(deprecated)]{
$arg.multiple_occurrences(true)
}})
({
if $arg.get_long().is_none() && $arg.get_short().is_none() {
$arg.multiple_values(true)
} else if $arg.is_takes_value_set() {
$arg.action($crate::ArgAction::Append)
} else {
$arg.action($crate::ArgAction::Count)
}
})
$($tail)*
}
};
Expand Down Expand Up @@ -493,7 +475,7 @@ macro_rules! arg_impl {
/// `...` (three consecutive dots/periods) specifies that this argument may occur multiple
/// times (not to be confused with multiple values per occurrence).
///
/// See [`Arg::multiple_occurrences`][crate::Arg::multiple_occurrences].
/// See [`ArgAction::Append`][crate::ArgAction::Append].
///
/// ### Help String
///
Expand Down
36 changes: 13 additions & 23 deletions tests/builder/groups.rs
View file
Expand Up @@ -86,9 +86,9 @@ fn arg_group_new_of_arg_name() {
#[test]
fn group_single_value() {
let res = Command::new("group")
.arg(arg!(-f --flag "some flag"))
.arg(arg!(-c --color [color] "some option"))
.group(ArgGroup::new("grp").args(&["flag", "color"]))
.arg(arg!(-h --hostname <name> "another option").required(false))
.group(ArgGroup::new("grp").args(&["hostname", "color"]))
.try_get_matches_from(vec!["", "-c", "blue"]);
assert!(res.is_ok(), "{}", res.unwrap_err());

Expand All @@ -100,26 +100,12 @@ fn group_single_value() {
);
}

#[test]
fn group_single_flag() {
let res = Command::new("group")
.arg(arg!(-f --flag "some flag"))
.arg(arg!(-c --color [color] "some option"))
.group(ArgGroup::new("grp").args(&["flag", "color"]))
.try_get_matches_from(vec!["", "-f"]);
assert!(res.is_ok(), "{}", res.unwrap_err());

let m = res.unwrap();
assert!(m.contains_id("grp"));
assert!(m.get_one::<String>("grp").map(|v| v.as_str()).is_none());
}

#[test]
fn group_empty() {
let res = Command::new("group")
.arg(arg!(-f --flag "some flag"))
.arg(arg!(-c --color [color] "some option"))
.group(ArgGroup::new("grp").args(&["flag", "color"]))
.arg(arg!(-h --hostname <name> "another option").required(false))
.group(ArgGroup::new("grp").args(&["hostname", "color"]))
.try_get_matches_from(vec![""]);
assert!(res.is_ok(), "{}", res.unwrap_err());

Expand All @@ -129,11 +115,15 @@ fn group_empty() {
}

#[test]
fn group_reqired_flags_empty() {
fn group_required_flags_empty() {
let result = Command::new("group")
.arg(arg!(-f --flag "some flag"))
.arg(arg!(-c --color "some option"))
.group(ArgGroup::new("grp").required(true).args(&["flag", "color"]))
.arg(arg!(-h --hostname <name> "another option").required(false))
.group(
ArgGroup::new("grp")
.required(true)
.args(&["hostname", "color"]),
)
.try_get_matches_from(vec![""]);
assert!(result.is_err());
let err = result.err().unwrap();
Expand All @@ -143,9 +133,9 @@ fn group_reqired_flags_empty() {
#[test]
fn group_multi_value_single_arg() {
let res = Command::new("group")
.arg(arg!(-f --flag "some flag"))
.arg(arg!(-c --color <color> "some option").multiple_values(true))
.group(ArgGroup::new("grp").args(&["flag", "color"]))
.arg(arg!(-h --hostname <name> "another option").required(false))
.group(ArgGroup::new("grp").args(&["hostname", "color"]))
.try_get_matches_from(vec!["", "-c", "blue", "red", "green"]);
assert!(res.is_ok(), "{:?}", res.unwrap_err().kind());

Expand Down
6 changes: 3 additions & 3 deletions tests/builder/help.rs
View file
Expand Up @@ -1731,7 +1731,7 @@ fn issue_1052_require_delim_help() {
.about("tests stuff")
.version("1.3")
.arg(
arg!(-f --fake "some help")
arg!(-f --fake <s> "some help")
.required(true)
.value_names(&["some", "val"])
.takes_value(true)
Expand All @@ -1750,7 +1750,7 @@ fn custom_headers_headers() {
.about("does stuff")
.version("1.4")
.arg(
arg!(-f --fake "some help")
arg!(-f --fake <s> "some help")
.required(true)
.value_names(&["some", "val"])
.takes_value(true)
Expand Down Expand Up @@ -1802,7 +1802,7 @@ fn multiple_custom_help_headers() {
.about("does stuff")
.version("1.4")
.arg(
arg!(-f --fake "some help")
arg!(-f --fake <s> "some help")
.required(true)
.value_names(&["some", "val"])
.takes_value(true)
Expand Down
4 changes: 2 additions & 2 deletions tests/builder/utils.rs
View file
Expand Up @@ -72,10 +72,10 @@ pub fn complex_app() -> Command<'static> {
.required(false)
.value_parser(opt3_vals),
arg!([positional3] ... "tests specific values").value_parser(pos3_vals),
arg!(--multvals "Tests multiple values, not mult occs")
arg!(--multvals <val> "Tests multiple values, not mult occs")
.value_names(&["one", "two"])
.required(false),
arg!(--multvalsmo ... "Tests multiple values, and mult occs")
arg!(--multvalsmo <val> ... "Tests multiple values, and mult occs")
.value_names(&["one", "two"])
.required(false),
arg!(--minvals2 <minvals> "Tests 2 min vals")
Expand Down

0 comments on commit 04cdfb9

Please sign in to comment.