From d3e36b1c901e4f589ef4564c53a1e6c299945531 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 4 May 2022 15:38:06 -0500 Subject: [PATCH] fix(v4): Disallow leading dashes in long's This is a step towards #3309. We want to make longs and long aliases more consistent in how they handle leading dashes. There is more flexibility offered in not stripping and it matches the v3 short behavior of only taking the non-dash form. This starts the process by disallowing it completely so people will catch problems with it and remove their existing leading dashes. In a subsequent breaking release we can remove the debug assert and allow triple-leading dashes. --- clap_complete/tests/common.rs | 4 ++-- clap_complete_fig/tests/common.rs | 4 ++-- clap_mangen/tests/common.rs | 8 ++++---- src/bin/stdio-fixture.rs | 2 +- src/build/arg.rs | 13 ++++++++++--- src/build/command.rs | 9 ++++++++- src/build/debug_asserts.rs | 8 ++++++++ tests/builder/flags.rs | 17 +++++++++++++++++ tests/builder/possible_values.rs | 20 ++++++++++---------- tests/builder/unicode.rs | 2 +- 10 files changed, 63 insertions(+), 24 deletions(-) diff --git a/clap_complete/tests/common.rs b/clap_complete/tests/common.rs index 3fd22100478..f68e3de1a75 100644 --- a/clap_complete/tests/common.rs +++ b/clap_complete/tests/common.rs @@ -48,7 +48,7 @@ pub fn special_commands_command(name: &'static str) -> clap::Command<'static> { .about("tests other things") .arg( clap::Arg::new("config") - .long("--config") + .long("config") .hide(true) .takes_value(true) .require_equals(true) @@ -139,7 +139,7 @@ pub fn sub_subcommands_command(name: &'static str) -> clap::Command<'static> { .subcommand( clap::Command::new("sub_cmd").about("sub-subcommand").arg( clap::Arg::new("config") - .long("--config") + .long("config") .takes_value(true) .possible_values([clap::PossibleValue::new("Lest quotes aren't escaped.")]) .help("the other case to test"), diff --git a/clap_complete_fig/tests/common.rs b/clap_complete_fig/tests/common.rs index 3fd22100478..f68e3de1a75 100644 --- a/clap_complete_fig/tests/common.rs +++ b/clap_complete_fig/tests/common.rs @@ -48,7 +48,7 @@ pub fn special_commands_command(name: &'static str) -> clap::Command<'static> { .about("tests other things") .arg( clap::Arg::new("config") - .long("--config") + .long("config") .hide(true) .takes_value(true) .require_equals(true) @@ -139,7 +139,7 @@ pub fn sub_subcommands_command(name: &'static str) -> clap::Command<'static> { .subcommand( clap::Command::new("sub_cmd").about("sub-subcommand").arg( clap::Arg::new("config") - .long("--config") + .long("config") .takes_value(true) .possible_values([clap::PossibleValue::new("Lest quotes aren't escaped.")]) .help("the other case to test"), diff --git a/clap_mangen/tests/common.rs b/clap_mangen/tests/common.rs index 27a2e951cf6..9d8ee45b166 100644 --- a/clap_mangen/tests/common.rs +++ b/clap_mangen/tests/common.rs @@ -45,7 +45,7 @@ pub fn special_commands_command(name: &'static str) -> clap::Command<'static> { .about("tests other things") .arg( clap::Arg::new("config") - .long("--config") + .long("config") .takes_value(true) .help("the other case to test"), ), @@ -128,7 +128,7 @@ pub fn sub_subcommands_command(name: &'static str) -> clap::Command<'static> { .subcommand( clap::Command::new("sub_cmd").about("sub-subcommand").arg( clap::Arg::new("config") - .long("--config") + .long("config") .takes_value(true) .possible_values([clap::PossibleValue::new("Lest quotes aren't escaped.")]) .help("the other case to test"), @@ -222,10 +222,10 @@ pub fn value_hint_command(name: &'static str) -> clap::Command<'static> { pub fn hidden_option_command(name: &'static str) -> clap::Command<'static> { clap::Command::new(name) - .arg(clap::Arg::new("config").long("--config").takes_value(true)) + .arg(clap::Arg::new("config").long("config").takes_value(true)) .arg( clap::Arg::new("no-config") - .long("--no-config") + .long("no-config") .hide(true) .overrides_with("config"), ) diff --git a/src/bin/stdio-fixture.rs b/src/bin/stdio-fixture.rs index bb04cada8f2..e3f34b41a22 100644 --- a/src/bin/stdio-fixture.rs +++ b/src/bin/stdio-fixture.rs @@ -6,7 +6,7 @@ fn main() { .subcommand(clap::Command::new("more")) .arg( clap::Arg::new("verbose") - .long("--verbose") + .long("verbose") .help("log") .long_help("more log"), ); diff --git a/src/build/arg.rs b/src/build/arg.rs index 6f73b4c02c2..80c5199dfb8 100644 --- a/src/build/arg.rs +++ b/src/build/arg.rs @@ -200,7 +200,14 @@ impl<'help> Arg<'help> { #[inline] #[must_use] pub fn long(mut self, l: &'help str) -> Self { - self.long = Some(l.trim_start_matches(|c| c == '-')); + #[cfg(feature = "unstable-v4")] + { + self.long = Some(l); + } + #[cfg(not(feature = "unstable-v4"))] + { + self.long = Some(l.trim_start_matches(|c| c == '-')); + } self } @@ -1819,7 +1826,7 @@ impl<'help> Arg<'help> { /// # use clap::{Command, Arg}; /// let m = Command::new("pv") /// .arg(Arg::new("option") - /// .long("--option") + /// .long("option") /// .takes_value(true) /// .ignore_case(true) /// .possible_value("test123")) @@ -1837,7 +1844,7 @@ impl<'help> Arg<'help> { /// let m = Command::new("pv") /// .arg(Arg::new("option") /// .short('o') - /// .long("--option") + /// .long("option") /// .takes_value(true) /// .ignore_case(true) /// .multiple_values(true) diff --git a/src/build/command.rs b/src/build/command.rs index 097872c1619..4ba7c844727 100644 --- a/src/build/command.rs +++ b/src/build/command.rs @@ -2263,7 +2263,14 @@ impl<'help> App<'help> { /// [`Arg::long`]: Arg::long() #[must_use] pub fn long_flag(mut self, long: &'help str) -> Self { - self.long_flag = Some(long.trim_start_matches(|c| c == '-')); + #[cfg(feature = "unstable-v4")] + { + self.long_flag = Some(long); + } + #[cfg(not(feature = "unstable-v4"))] + { + self.long_flag = Some(long.trim_start_matches(|c| c == '-')); + } self } diff --git a/src/build/debug_asserts.rs b/src/build/debug_asserts.rs index 1bbf86a6130..90f1b7e0d91 100644 --- a/src/build/debug_asserts.rs +++ b/src/build/debug_asserts.rs @@ -45,6 +45,10 @@ pub(crate) fn assert_app(cmd: &Command) { } if let Some(l) = sc.get_long_flag().as_ref() { + #[cfg(feature = "unstable-v4")] + { + assert!(!l.starts_with('-'), "Command {}: long_flag {:?} must not start with a `-`, that will be handled by the parser", sc.get_name(), l); + } long_flags.push(Flag::Command(format!("--{}", l), sc.get_name())); } @@ -75,6 +79,10 @@ pub(crate) fn assert_app(cmd: &Command) { } if let Some(l) = arg.long.as_ref() { + #[cfg(feature = "unstable-v4")] + { + assert!(!l.starts_with('-'), "Argument {}: long {:?} must not start with a `-`, that will be handled by the parser", arg.name, l); + } long_flags.push(Flag::Arg(format!("--{}", l), &*arg.name)); } diff --git a/tests/builder/flags.rs b/tests/builder/flags.rs index 343cc3a979b..fd8a4301b44 100644 --- a/tests/builder/flags.rs +++ b/tests/builder/flags.rs @@ -179,3 +179,20 @@ For more information try --help utils::assert_output(cmd, "test -----", MULTIPLE_DASHES, true); } + +#[test] +#[cfg(not(feature = "unstable-v4"))] +fn leading_dash_stripped() { + let cmd = Command::new("mycat").arg(Arg::new("filename").long("--filename")); + let matches = cmd.try_get_matches_from(["mycat", "--filename"]).unwrap(); + assert!(matches.is_present("filename")); +} + +#[test] +#[cfg(feature = "unstable-v4")] +#[cfg(debug_assertions)] +#[should_panic = "Argument filename: long \"--filename\" must not start with a `-`, that will be handled by the parser"] +fn leading_dash_stripped() { + let cmd = Command::new("mycat").arg(Arg::new("filename").long("--filename")); + cmd.debug_assert(); +} diff --git a/tests/builder/possible_values.rs b/tests/builder/possible_values.rs index 58e6bd55fc8..ccf17239732 100644 --- a/tests/builder/possible_values.rs +++ b/tests/builder/possible_values.rs @@ -134,7 +134,7 @@ fn possible_values_of_option() { .arg( Arg::new("option") .short('o') - .long("--option") + .long("option") .takes_value(true) .possible_value("test123"), ) @@ -153,7 +153,7 @@ fn possible_values_of_option_fail() { .arg( Arg::new("option") .short('o') - .long("--option") + .long("option") .takes_value(true) .possible_value("test123"), ) @@ -169,7 +169,7 @@ fn possible_values_of_option_multiple() { .arg( Arg::new("option") .short('o') - .long("--option") + .long("option") .takes_value(true) .possible_value("test123") .possible_value("test321") @@ -193,7 +193,7 @@ fn possible_values_of_option_multiple_fail() { .arg( Arg::new("option") .short('o') - .long("--option") + .long("option") .takes_value(true) .possible_value("test123") .possible_value("test321") @@ -298,7 +298,7 @@ fn alias() { .arg( Arg::new("option") .short('o') - .long("--option") + .long("option") .takes_value(true) .possible_value(PossibleValue::new("test123").alias("123")) .possible_value("test321") @@ -316,7 +316,7 @@ fn aliases() { .arg( Arg::new("option") .short('o') - .long("--option") + .long("option") .takes_value(true) .possible_value(PossibleValue::new("test123").aliases(["1", "2", "3"])) .possible_value("test321") @@ -334,7 +334,7 @@ fn ignore_case() { .arg( Arg::new("option") .short('o') - .long("--option") + .long("option") .takes_value(true) .possible_value("test123") .possible_value("test321") @@ -356,7 +356,7 @@ fn ignore_case_fail() { .arg( Arg::new("option") .short('o') - .long("--option") + .long("option") .takes_value(true) .possible_value("test123") .possible_value("test321"), @@ -373,7 +373,7 @@ fn ignore_case_multiple() { .arg( Arg::new("option") .short('o') - .long("--option") + .long("option") .takes_value(true) .possible_value("test123") .possible_value("test321") @@ -395,7 +395,7 @@ fn ignore_case_multiple_fail() { .arg( Arg::new("option") .short('o') - .long("--option") + .long("option") .takes_value(true) .possible_value("test123") .possible_value("test321") diff --git a/tests/builder/unicode.rs b/tests/builder/unicode.rs index e3de1b1856b..dc97787b246 100644 --- a/tests/builder/unicode.rs +++ b/tests/builder/unicode.rs @@ -6,7 +6,7 @@ fn possible_values_ignore_case() { .arg( clap::Arg::new("option") .short('o') - .long("--option") + .long("option") .takes_value(true) .possible_value("รค") .ignore_case(true),