From dffd7932b333d269bc8769e2fd0b4584b071c537 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Jun 2022 11:58:15 -0500 Subject: [PATCH 1/3] fix(assert): Check for version if user specifies ArgAction::Version --- src/builder/debug_asserts.rs | 23 ++++++++++------- tests/builder/app_settings.rs | 44 +++++++++++++++++++-------------- tests/builder/legacy/version.rs | 2 +- tests/builder/version.rs | 9 +++---- 4 files changed, 44 insertions(+), 34 deletions(-) diff --git a/src/builder/debug_asserts.rs b/src/builder/debug_asserts.rs index 91ab7dfe54c..6a3041e7d94 100644 --- a/src/builder/debug_asserts.rs +++ b/src/builder/debug_asserts.rs @@ -4,8 +4,8 @@ use clap_lex::RawOsStr; use crate::builder::arg::ArgProvider; use crate::mkeymap::KeyType; -use crate::util::Id; -use crate::{AppSettings, Arg, Command, ValueHint}; +use crate::ArgAction; +use crate::{Arg, Command, ValueHint}; pub(crate) fn assert_app(cmd: &Command) { debug!("Command::_debug_asserts"); @@ -23,16 +23,21 @@ pub(crate) fn assert_app(cmd: &Command) { ); // Used `Command::mut_arg("version", ..) but did not provide any version information to display - let has_mutated_version = cmd + let version_needed = cmd .get_arguments() - .any(|x| x.id == Id::version_hash() && x.provider == ArgProvider::GeneratedMutated); + .filter(|x| { + matches!(x.get_action(), ArgAction::Version) + && matches!( + x.provider, + ArgProvider::User | ArgProvider::GeneratedMutated + ) + }) + .map(|x| x.get_id()) + .collect::>(); - if has_mutated_version { - assert!(cmd.is_set(AppSettings::NoAutoVersion), - "Command {}: Used Command::mut_arg(\"version\", ..) without providing Command::version, Command::long_version or using AppSettings::NoAutoVersion" + assert_eq!(version_needed, Vec::<&str>::new(), "Command {}: `ArgAction::Version` used without providing Command::version or Command::long_version" ,cmd.get_name() - ); - } + ); } for sc in cmd.get_subcommands() { diff --git a/tests/builder/app_settings.rs b/tests/builder/app_settings.rs index 34f8f6efc27..4c02be758a6 100644 --- a/tests/builder/app_settings.rs +++ b/tests/builder/app_settings.rs @@ -2,7 +2,7 @@ use std::ffi::OsString; use super::utils; -use clap::{arg, error::ErrorKind, AppSettings, Arg, Command}; +use clap::{arg, error::ErrorKind, AppSettings, Arg, ArgAction, Command}; static ALLOW_EXT_SC: &str = "clap-test v1.4.8 @@ -1345,62 +1345,68 @@ fn aaos_option_use_delim_false() { #[test] fn no_auto_help() { let cmd = Command::new("myprog") - .setting(AppSettings::NoAutoHelp) - .subcommand(Command::new("foo")); + .subcommand(Command::new("foo")) + .mut_arg("help", |v| v.action(ArgAction::SetTrue)); let result = cmd.clone().try_get_matches_from("myprog --help".split(' ')); - assert!(result.is_ok(), "{}", result.unwrap_err()); - assert!(result.unwrap().is_present("help")); + assert_eq!(result.unwrap().get_one::("help").copied(), Some(true)); let result = cmd.clone().try_get_matches_from("myprog -h".split(' ')); - assert!(result.is_ok(), "{}", result.unwrap_err()); - assert!(result.unwrap().is_present("help")); - - let result = cmd.clone().try_get_matches_from("myprog help".split(' ')); - - assert!(result.is_ok(), "{}", result.unwrap_err()); - assert_eq!(result.unwrap().subcommand_name(), Some("help")); + assert_eq!(result.unwrap().get_one::("help").copied(), Some(true)); } #[test] fn no_auto_version() { let cmd = Command::new("myprog") .version("3.0") - .setting(AppSettings::NoAutoVersion); + .mut_arg("version", |v| v.action(ArgAction::SetTrue)); let result = cmd .clone() .try_get_matches_from("myprog --version".split(' ')); assert!(result.is_ok(), "{}", result.unwrap_err()); - assert!(result.unwrap().is_present("version")); + assert_eq!( + result.unwrap().get_one::("version").copied(), + Some(true) + ); let result = cmd.clone().try_get_matches_from("myprog -V".split(' ')); assert!(result.is_ok(), "{}", result.unwrap_err()); - assert!(result.unwrap().is_present("version")); + assert_eq!( + result.unwrap().get_one::("version").copied(), + Some(true) + ); } #[test] fn no_auto_version_mut_arg() { let cmd = Command::new("myprog") .version("3.0") - .mut_arg("version", |v| v.help("custom help")) - .setting(AppSettings::NoAutoVersion); + .mut_arg("version", |v| { + v.action(ArgAction::SetTrue).help("custom help") + }); let result = cmd .clone() .try_get_matches_from("myprog --version".split(' ')); assert!(result.is_ok(), "{}", result.unwrap_err()); - assert!(result.unwrap().is_present("version")); + assert_eq!( + result.unwrap().get_one::("version").copied(), + Some(true) + ); let result = cmd.clone().try_get_matches_from("myprog -V".split(' ')); assert!(result.is_ok(), "{}", result.unwrap_err()); - assert!(result.unwrap().is_present("version")); + assert_eq!( + result.unwrap().get_one::("version").copied(), + Some(true) + ); } #[test] diff --git a/tests/builder/legacy/version.rs b/tests/builder/legacy/version.rs index f80d3d77deb..55a7077ce67 100644 --- a/tests/builder/legacy/version.rs +++ b/tests/builder/legacy/version.rs @@ -206,7 +206,7 @@ fn propagate_version_short() { #[cfg(debug_assertions)] #[test] -#[should_panic = "Used Command::mut_arg(\"version\", ..) without providing Command::version, Command::long_version or using AppSettings::NoAutoVersion"] +#[should_panic = "`ArgAction::Version` used without providing Command::version or Command::long_version"] fn mut_arg_version_panic() { let _res = common() .mut_arg("version", |v| v.short('z')) diff --git a/tests/builder/version.rs b/tests/builder/version.rs index f80d3d77deb..7702359b0a9 100644 --- a/tests/builder/version.rs +++ b/tests/builder/version.rs @@ -2,7 +2,7 @@ use super::utils; use std::str; -use clap::{error::ErrorKind, AppSettings, Arg, Command}; +use clap::{error::ErrorKind, Arg, ArgAction, Command}; fn common() -> Command<'static> { Command::new("foo") @@ -206,7 +206,7 @@ fn propagate_version_short() { #[cfg(debug_assertions)] #[test] -#[should_panic = "Used Command::mut_arg(\"version\", ..) without providing Command::version, Command::long_version or using AppSettings::NoAutoVersion"] +#[should_panic = "`ArgAction::Version` used without providing Command::version or Command::long_version"] fn mut_arg_version_panic() { let _res = common() .mut_arg("version", |v| v.short('z')) @@ -216,12 +216,11 @@ fn mut_arg_version_panic() { #[test] fn mut_arg_version_no_auto_version() { let res = common() - .mut_arg("version", |v| v.short('z')) - .setting(AppSettings::NoAutoVersion) + .mut_arg("version", |v| v.short('z').action(ArgAction::SetTrue)) .try_get_matches_from("foo -z".split(' ')); assert!(res.is_ok(), "{}", res.unwrap_err()); - assert!(res.unwrap().is_present("version")); + assert_eq!(res.unwrap().get_one::("version").copied(), Some(true)); } #[cfg(debug_assertions)] From 28781d677315d1fac3a30cc4c9c2dad976cfed0f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Jun 2022 12:02:10 -0500 Subject: [PATCH 2/3] fix(help): Deprecate NoAutoVersion/NoAutoHelp I'm a bit disappointed we don't have a way to control the action for the help subcommand. Instead, people will need to provide their own and disable ours. Long term, I want to design actions for subcommands but I don't think its worth keeping `NoAutoHelp` around for it. --- src/builder/app_settings.rs | 43 ++++--------------------------------- src/parser/parser.rs | 1 + 2 files changed, 5 insertions(+), 39 deletions(-) diff --git a/src/builder/app_settings.rs b/src/builder/app_settings.rs index d16ffc336df..61b1c18c93e 100644 --- a/src/builder/app_settings.rs +++ b/src/builder/app_settings.rs @@ -293,47 +293,12 @@ pub enum AppSettings { #[deprecated(since = "3.1.0", note = "Replaced with `Command::no_binary_name`")] NoBinaryName, - /// Treat the auto-generated `-h, --help` flags like any other flag, and *not* print the help - /// message. - /// - /// This allows one to handle printing of the help message manually. - /// - /// ```rust - /// # use clap::{Command, AppSettings}; - /// let result = Command::new("myprog") - /// .setting(AppSettings::NoAutoHelp) - /// .try_get_matches_from("myprog --help".split(" ")); - /// - /// // Normally, if `--help` is used clap prints the help message and returns an - /// // ErrorKind::DisplayHelp - /// // - /// // However, `--help` was treated like a normal flag - /// - /// assert!(result.is_ok()); - /// assert!(result.unwrap().is_present("help")); - /// ``` + /// Deprecated, replaced with [`Arg::action`][super::Arg::action] + #[deprecated(since = "3.2.0", note = "Replaced with `Arg::action`")] NoAutoHelp, - /// Treat the auto-generated `-V, --version` flags like any other flag, and - /// *not* print the version message. - /// - /// This allows one to handle printing of the version message manually. - /// - /// ```rust - /// # use clap::{Command, AppSettings}; - /// let result = Command::new("myprog") - /// .version("3.0") - /// .setting(AppSettings::NoAutoVersion) - /// .try_get_matches_from("myprog --version".split(" ")); - /// - /// // Normally, if `--version` is used clap prints the version message and returns an - /// // ErrorKind::DisplayVersion - /// // - /// // However, `--version` was treated like a normal flag - /// - /// assert!(result.is_ok()); - /// assert!(result.unwrap().is_present("version")); - /// ``` + /// Deprecated, replaced with [`Arg::action`][super::Arg::action] + #[deprecated(since = "3.2.0", note = "Replaced with `Arg::action`")] NoAutoVersion, /// Deprecated, replaced with [`AppSettings::AllowHyphenValues`] diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 7b1d5130bf3..b1f365088ad 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -178,6 +178,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { let sc_name = self.possible_subcommand(arg_os.to_value(), valid_arg_found); debug!("Parser::get_matches_with: sc={:?}", sc_name); if let Some(sc_name) = sc_name { + #[allow(deprecated)] if sc_name == "help" && !self.is_set(AS::NoAutoHelp) && !self.cmd.is_disable_help_subcommand_set() From 10bb9abb1a260b125b6117431c01a37e30ca2cd6 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 8 Jun 2022 12:06:15 -0500 Subject: [PATCH 3/3] fix(assert): Reference help_expected --- src/builder/command.rs | 2 +- tests/builder/help.rs | 8 ++++---- tests/builder/legacy/help.rs | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/builder/command.rs b/src/builder/command.rs index 26acde48182..0a52a645816 100644 --- a/src/builder/command.rs +++ b/src/builder/command.rs @@ -4373,7 +4373,7 @@ impl<'help> App<'help> { .collect(); assert!(args_missing_help.is_empty(), - "AppSettings::HelpExpected is enabled for the Command {}, but at least one of its arguments does not have either `help` or `long_help` set. List of such arguments: {}", + "Command::help_expected is enabled for the Command {}, but at least one of its arguments does not have either `help` or `long_help` set. List of such arguments: {}", self.name, args_missing_help.join(", ") ); diff --git a/tests/builder/help.rs b/tests/builder/help.rs index 6aaec5afee0..e95dfe4ef20 100644 --- a/tests/builder/help.rs +++ b/tests/builder/help.rs @@ -1997,7 +1997,7 @@ fn issue_1487() { #[cfg(debug_assertions)] #[test] -#[should_panic = "AppSettings::HelpExpected is enabled for the Command"] +#[should_panic = "Command::help_expected is enabled for the Command"] fn help_required_but_not_given() { Command::new("myapp") .help_expected(true) @@ -2008,7 +2008,7 @@ fn help_required_but_not_given() { #[cfg(debug_assertions)] #[test] -#[should_panic = "AppSettings::HelpExpected is enabled for the Command"] +#[should_panic = "Command::help_expected is enabled for the Command"] fn help_required_but_not_given_settings_after_args() { Command::new("myapp") .arg(Arg::new("foo")) @@ -2019,7 +2019,7 @@ fn help_required_but_not_given_settings_after_args() { #[cfg(debug_assertions)] #[test] -#[should_panic = "AppSettings::HelpExpected is enabled for the Command"] +#[should_panic = "Command::help_expected is enabled for the Command"] fn help_required_but_not_given_for_one_of_two_arguments() { Command::new("myapp") .help_expected(true) @@ -2046,7 +2046,7 @@ fn help_required_globally() { #[cfg(debug_assertions)] #[test] -#[should_panic = "AppSettings::HelpExpected is enabled for the Command"] +#[should_panic = "Command::help_expected is enabled for the Command"] fn help_required_globally_but_not_given_for_subcommand() { Command::new("myapp") .help_expected(true) diff --git a/tests/builder/legacy/help.rs b/tests/builder/legacy/help.rs index 6701ed712fe..0a62007fd5d 100644 --- a/tests/builder/legacy/help.rs +++ b/tests/builder/legacy/help.rs @@ -1991,7 +1991,7 @@ fn issue_1487() { #[cfg(debug_assertions)] #[test] -#[should_panic = "AppSettings::HelpExpected is enabled for the Command"] +#[should_panic = "Command::help_expected is enabled for the Command"] fn help_required_but_not_given() { Command::new("myapp") .help_expected(true) @@ -2002,7 +2002,7 @@ fn help_required_but_not_given() { #[cfg(debug_assertions)] #[test] -#[should_panic = "AppSettings::HelpExpected is enabled for the Command"] +#[should_panic = "Command::help_expected is enabled for the Command"] fn help_required_but_not_given_settings_after_args() { Command::new("myapp") .arg(Arg::new("foo")) @@ -2013,7 +2013,7 @@ fn help_required_but_not_given_settings_after_args() { #[cfg(debug_assertions)] #[test] -#[should_panic = "AppSettings::HelpExpected is enabled for the Command"] +#[should_panic = "Command::help_expected is enabled for the Command"] fn help_required_but_not_given_for_one_of_two_arguments() { Command::new("myapp") .help_expected(true) @@ -2040,7 +2040,7 @@ fn help_required_globally() { #[cfg(debug_assertions)] #[test] -#[should_panic = "AppSettings::HelpExpected is enabled for the Command"] +#[should_panic = "Command::help_expected is enabled for the Command"] fn help_required_globally_but_not_given_for_subcommand() { Command::new("myapp") .help_expected(true)