Skip to content

Commit

Permalink
fix: No implicit version/help actions
Browse files Browse the repository at this point in the history 
Documenting the existing behavior is challenging which suggests it can
cause user confusion.  So long as its not too hard to explicitly
specify actions, we should just do it.

Fixes clap-rs#4057
  • Loading branch information
@epage
epage committed Aug 11, 2022
1 parent 1a08b91 commit 226ef47
Show file tree
Hide file tree
Showing 7 changed files with 87 additions and 67 deletions.
12 changes: 1 addition & 11 deletions src/builder/arg.rs
View file
Expand Up @@ -3896,17 +3896,7 @@ impl<'help> Arg<'help> {
impl<'help> Arg<'help> {
pub(crate) fn _build(&mut self) {
if self.action.is_none() {
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"
&& matches!(self.get_num_args(), Some(ValueRange::EMPTY) | None)
{
let action = super::ArgAction::Version;
self.action = Some(action);
} else if self.num_vals == Some(ValueRange::EMPTY) {
if self.num_vals == Some(ValueRange::EMPTY) {
let action = super::ArgAction::SetTrue;
self.action = Some(action);
} else {
Expand Down
24 changes: 4 additions & 20 deletions src/macros.rs
View file
Expand Up @@ -207,11 +207,7 @@ macro_rules! arg_impl {
if arg.get_id().is_empty() {
arg = arg.id(long);
}
let action = match arg.get_id() {
"help" => $crate::ArgAction::Help,
"version" => $crate::ArgAction::Version,
_ => $crate::ArgAction::SetTrue,
};
let action = $crate::ArgAction::SetTrue;
arg
.long(long)
.action(action)
Expand All @@ -236,11 +232,7 @@ macro_rules! arg_impl {
if arg.get_id().is_empty() {
arg = arg.id(long);
}
let action = match arg.get_id() {
"help" => $crate::ArgAction::Help,
"version" => $crate::ArgAction::Version,
_ => $crate::ArgAction::SetTrue,
};
let action = $crate::ArgAction::SetTrue;
arg
.long(long)
.action(action)
Expand All @@ -261,11 +253,7 @@ macro_rules! arg_impl {
debug_assert_eq!($arg.get_value_names(), None, "Flags should precede values");
debug_assert!(!matches!($arg.get_action(), $crate::ArgAction::Append), "Flags should precede `...`");

let action = match $arg.get_id() {
"help" => $crate::ArgAction::Help,
"version" => $crate::ArgAction::Version,
_ => $crate::ArgAction::SetTrue,
};
let action = $crate::ArgAction::SetTrue;
$arg
.short($crate::arg_impl! { @char $short })
.action(action)
Expand All @@ -286,11 +274,7 @@ macro_rules! arg_impl {
debug_assert_eq!($arg.get_value_names(), None, "Flags should precede values");
debug_assert!(!matches!($arg.get_action(), $crate::ArgAction::Append), "Flags should precede `...`");

let action = match $arg.get_id() {
"help" => $crate::ArgAction::Help,
"version" => $crate::ArgAction::Version,
_ => $crate::ArgAction::SetTrue,
};
let action = $crate::ArgAction::SetTrue;
$arg
.short($crate::arg_impl! { @char $short })
.action(action)
Expand Down
10 changes: 5 additions & 5 deletions tests/builder/help.rs
View file
Expand Up @@ -1248,7 +1248,7 @@ OPTIONS:
fn override_help_short() {
let cmd = Command::new("test")
.version("0.1")
.arg(arg!(-H --help "Print help information"))
.arg(arg!(-H --help "Print help information").action(ArgAction::Help))
.disable_help_flag(true);

utils::assert_output(cmd.clone(), "test --help", OVERRIDE_HELP_SHORT, false);
Expand Down Expand Up @@ -1290,7 +1290,7 @@ OPTIONS:
fn override_help_about() {
let cmd = Command::new("test")
.version("0.1")
.arg(arg!(-h --help "Print custom help information"))
.arg(arg!(-h --help "Print custom help information").action(ArgAction::Help))
.disable_help_flag(true);

utils::assert_output(cmd.clone(), "test --help", OVERRIDE_HELP_ABOUT, false);
Expand Down Expand Up @@ -2237,7 +2237,7 @@ fn only_custom_heading_opts_no_args() {
.version("1.4")
.disable_version_flag(true)
.disable_help_flag(true)
.arg(arg!(--help).hide(true))
.arg(arg!(--help).action(ArgAction::Help).hide(true))
.next_help_heading(Some("NETWORKING"))
.arg(arg!(-s --speed <SPEED> "How fast").required(false));

Expand All @@ -2259,7 +2259,7 @@ fn only_custom_heading_pos_no_args() {
.version("1.4")
.disable_version_flag(true)
.disable_help_flag(true)
.arg(arg!(--help).hide(true))
.arg(arg!(--help).action(ArgAction::Help).hide(true))
.next_help_heading(Some("NETWORKING"))
.arg(Arg::new("speed").help("How fast"));

Expand Down Expand Up @@ -2624,7 +2624,7 @@ ARGS:
fn help_without_short() {
let mut cmd = clap::Command::new("test")
.arg(arg!(-h --hex <NUM>))
.arg(arg!(--help))
.arg(arg!(--help).action(ArgAction::Help))
.disable_help_flag(true);

cmd.build();
Expand Down
6 changes: 3 additions & 3 deletions tests/builder/hidden_args.rs
View file
Expand Up @@ -248,7 +248,7 @@ fn hide_opt_args_only() {
.after_help("After help")
.disable_help_flag(true)
.disable_version_flag(true)
.arg(arg!(-h - -help).hide(true))
.arg(arg!(-h - -help).action(ArgAction::Help).hide(true))
.arg(arg!(-v - -version).hide(true))
.arg(
arg!(--option <opt> "some option")
Expand All @@ -274,7 +274,7 @@ fn hide_pos_args_only() {
.after_help("After help")
.disable_help_flag(true)
.disable_version_flag(true)
.arg(arg!(-h - -help).hide(true))
.arg(arg!(-h - -help).action(ArgAction::Help).hide(true))
.arg(arg!(-v - -version).hide(true))
.args(&[Arg::new("pos").help("some pos").hide(true)]);

Expand All @@ -296,7 +296,7 @@ fn hide_subcmds_only() {
.after_help("After help")
.disable_help_flag(true)
.disable_version_flag(true)
.arg(arg!(-h - -help).hide(true))
.arg(arg!(-h - -help).action(ArgAction::Help).hide(true))
.arg(arg!(-v - -version).hide(true))
.subcommand(Command::new("sub").hide(true));

Expand Down
6 changes: 3 additions & 3 deletions tests/builder/version.rs
View file
@@ -1,4 +1,4 @@
use clap::{error::ErrorKind, ArgAction, Command};
use clap::{arg, error::ErrorKind, ArgAction, Command};

fn common() -> Command<'static> {
Command::new("foo")
Expand Down Expand Up @@ -140,9 +140,9 @@ fn propagate_version_short() {
#[cfg(debug_assertions)]
#[test]
#[should_panic = "`ArgAction::Version` used without providing Command::version or Command::long_version"]
fn mut_arg_version_panic() {
fn version_required() {
let _res = common()
.mut_arg("version", |v| v.short('z'))
.arg(arg!(--version).action(ArgAction::Version))
.try_get_matches_from("foo -z".split(' '));
}

Expand Down
30 changes: 29 additions & 1 deletion tests/derive/help.rs
View file
@@ -1,4 +1,4 @@
use clap::{Args, CommandFactory, Parser, Subcommand};
use clap::{ArgAction, Args, CommandFactory, Parser, Subcommand};

#[test]
fn arg_help_heading_applied() {
Expand Down Expand Up @@ -441,3 +441,31 @@ OPTIONS:
let help = String::from_utf8(buffer).unwrap();
snapbox::assert_eq(HELP, help);
}

#[test]
fn custom_help_flag() {
#[derive(Debug, Clone, Parser)]
#[clap(disable_help_flag = true)]
struct CliOptions {
#[clap(short = 'h', long = "verbose-help", action = ArgAction::Help)]
help: bool,
}

let result = CliOptions::try_parse_from(["cmd", "--verbose-help"]);
let err = result.unwrap_err();
assert_eq!(err.kind(), clap::error::ErrorKind::DisplayHelp);
}

#[test]
fn custom_version_flag() {
#[derive(Debug, Clone, Parser)]
#[clap(disable_version_flag = true, version = "2.0.0")]
struct CliOptions {
#[clap(short = 'V', long = "verbose-version", action = ArgAction::Version)]
version: bool,
}

let result = CliOptions::try_parse_from(["cmd", "--verbose-version"]);
let err = result.unwrap_err();
assert_eq!(err.kind(), clap::error::ErrorKind::DisplayVersion);
}
66 changes: 42 additions & 24 deletions tests/macros.rs
View file
Expand Up @@ -115,49 +115,67 @@ mod arg {
#[test]
fn short_help() {
let arg = clap::arg!(help: -b);
assert!(matches!(arg.get_action(), clap::ArgAction::Help));

let arg = clap::arg!(help: -b ...);
assert!(matches!(arg.get_action(), clap::ArgAction::Count));
assert!(matches!(arg.get_action(), clap::ArgAction::SetTrue));

let arg = clap::arg!(help: -b <NUM>);
assert!(matches!(arg.get_action(), clap::ArgAction::Set));
let mut cmd = clap::Command::new("cmd")
.disable_help_flag(true)
.arg(clap::arg!(help: -b).action(clap::ArgAction::Help));
cmd.build();
let arg = cmd
.get_arguments()
.find(|arg| arg.get_id() == "help")
.unwrap();
assert!(matches!(arg.get_action(), clap::ArgAction::Help));
}

#[test]
fn long_help() {
let arg = clap::arg!(-'?' - -help);
assert!(matches!(arg.get_action(), clap::ArgAction::Help));

let arg = clap::arg!(-'?' --help ...);
assert!(matches!(arg.get_action(), clap::ArgAction::Count));
assert!(matches!(arg.get_action(), clap::ArgAction::SetTrue));

let arg = clap::arg!(-'?' --help <NUM>);
assert!(matches!(arg.get_action(), clap::ArgAction::Set));
let mut cmd = clap::Command::new("cmd")
.disable_help_flag(true)
.arg(clap::arg!(-'?' - -help).action(clap::ArgAction::Help));
cmd.build();
let arg = cmd
.get_arguments()
.find(|arg| arg.get_id() == "help")
.unwrap();
assert!(matches!(arg.get_action(), clap::ArgAction::Help));
}

#[test]
fn short_version() {
let arg = clap::arg!(version: -b);
assert!(matches!(arg.get_action(), clap::ArgAction::Version));

let arg = clap::arg!(version: -b ...);
assert!(matches!(arg.get_action(), clap::ArgAction::Count));
assert!(matches!(arg.get_action(), clap::ArgAction::SetTrue));

let arg = clap::arg!(version: -b <NUM>);
assert!(matches!(arg.get_action(), clap::ArgAction::Set));
let mut cmd = clap::Command::new("cmd")
.disable_version_flag(true)
.version("1.0.0")
.arg(clap::arg!(version: -b).action(clap::ArgAction::Version));
cmd.build();
let arg = cmd
.get_arguments()
.find(|arg| arg.get_id() == "version")
.unwrap();
assert!(matches!(arg.get_action(), clap::ArgAction::Version));
}

#[test]
fn long_version() {
let arg = clap::arg!(-'?' - -version);
assert!(matches!(arg.get_action(), clap::ArgAction::Version));

let arg = clap::arg!(-'?' --version ...);
assert!(matches!(arg.get_action(), clap::ArgAction::Count));
assert!(matches!(arg.get_action(), clap::ArgAction::SetTrue));

let arg = clap::arg!(-'?' --version <NUM>);
assert!(matches!(arg.get_action(), clap::ArgAction::Set));
let mut cmd = clap::Command::new("cmd")
.disable_version_flag(true)
.version("1.0.0")
.arg(clap::arg!(-'?' - -version).action(clap::ArgAction::Version));
cmd.build();
let arg = cmd
.get_arguments()
.find(|arg| arg.get_id() == "version")
.unwrap();
assert!(matches!(arg.get_action(), clap::ArgAction::Version));
}

#[test]
Expand Down

0 comments on commit 226ef47

Please sign in to comment.