From 2d2db652efa3354cbb48a1cc28b7b05aa3ec308d Mon Sep 17 00:00:00 2001 From: rami3l Date: Sat, 24 Jul 2021 22:58:42 +0200 Subject: [PATCH 01/11] fix: fix #1649, take 1 --- src/parse/parser.rs | 46 ++++++++++++++++++++++++++++++++++++++++++++- tests/flags.rs | 26 +++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 04dc6c96137..3e49ef22c7d 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -1075,10 +1075,47 @@ impl<'help, 'app> Parser<'help, 'app> { self.app.settings.set(AS::ValidArgFound); self.seen.push(opt.id.clone()); if opt.is_set(ArgSettings::TakesValue) { + debug!( + "Parser::parse_long_arg: Found an opt with value '{:?}'", + &val + ); return self.parse_opt(&val, opt, matcher); } else { + debug!("Parser::parse_long_arg: Found a flag"); self.check_for_help_and_version_str(&arg)?; - return Ok(self.parse_flag(opt, matcher)); + debug!("Parser::parse_long_arg: Handle boolean literals for flags"); + let val = val.as_ref().and_then(|s| s.to_str()); + return match val.as_deref() { + Some("=true") | None => { + debug!("Parser::parse_long_arg: Got a `=true`"); + Ok(self.parse_flag(opt, matcher)) + } + Some("=false") => { + debug!("Parser::parse_long_arg: Got a `=false`"); + Ok(self.parse_flag_false(opt, matcher)) + } + Some(rest) => { + debug!("Parser::parse_long_arg: Got invalid literal `{}`", rest); + let used: Vec = matcher + .arg_names() + .filter(|&n| { + self.app.find(n).map_or(true, |a| { + !(a.is_set(ArgSettings::Hidden) + || self.required.contains(&a.id)) + }) + }) + .cloned() + .collect(); + + Err(ClapError::invalid_value( + rest.into(), + &["=true", "=false"], + opt, + Usage::new(self).create_usage_no_title(&used), + self.app.color(), + )) + } + }; } } @@ -1411,6 +1448,13 @@ impl<'help, 'app> Parser<'help, 'app> { ParseResult::Flag } + fn parse_flag_false(&self, flag: &Arg<'help>, matcher: &mut ArgMatcher) -> ParseResult { + debug!("Parser::parse_flag_false"); + // This should reset the number of occurrences. + matcher.remove(&flag.id); + ParseResult::Flag + } + fn remove_overrides(&mut self, matcher: &mut ArgMatcher) { debug!("Parser::remove_overrides"); let mut to_rem: Vec = Vec::new(); diff --git a/tests/flags.rs b/tests/flags.rs index 8502676ba01..a49429ef427 100644 --- a/tests/flags.rs +++ b/tests/flags.rs @@ -87,6 +87,32 @@ fn flag_using_long() { assert!(m.is_present("color")); } +#[test] +fn flag_using_long_with_literals() { + let m = App::new("flag") + .args(&[ + Arg::from("--flag 'some flag'"), + Arg::from("--color 'some other flag'"), + Arg::from("--sky 'a third flag'"), + Arg::from("--rainbow 'yet another flag'").multiple_occurrences(true), + ]) + .get_matches_from(vec![ + "", + "--flag=true", + "--color=false", + "--sky", + "--rainbow", + "--rainbow=false", // Clears the previous occurrence. + "--rainbow", + "--rainbow=true", + ]); + assert!(m.is_present("flag")); + assert!(!m.is_present("color")); + assert!(m.is_present("sky")); + assert!(m.is_present("rainbow")); + assert_eq!(m.occurrences_of("rainbow"), 2); +} + #[test] fn flag_using_mixed() { let m = App::new("flag") From 87479dfb8cd3cbeeeab35ace0fa72a3391b26b45 Mon Sep 17 00:00:00 2001 From: rami3l Date: Wed, 28 Jul 2021 14:30:10 +0200 Subject: [PATCH 02/11] fix: ban all uses of literals with flags --- src/parse/parser.rs | 70 +++++++++++++++++---------------------------- tests/flags.rs | 24 ++++++---------- 2 files changed, 35 insertions(+), 59 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 3e49ef22c7d..72ac26d313f 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -1080,43 +1080,34 @@ impl<'help, 'app> Parser<'help, 'app> { &val ); return self.parse_opt(&val, opt, matcher); - } else { - debug!("Parser::parse_long_arg: Found a flag"); - self.check_for_help_and_version_str(&arg)?; - debug!("Parser::parse_long_arg: Handle boolean literals for flags"); - let val = val.as_ref().and_then(|s| s.to_str()); - return match val.as_deref() { - Some("=true") | None => { - debug!("Parser::parse_long_arg: Got a `=true`"); - Ok(self.parse_flag(opt, matcher)) - } - Some("=false") => { - debug!("Parser::parse_long_arg: Got a `=false`"); - Ok(self.parse_flag_false(opt, matcher)) - } - Some(rest) => { - debug!("Parser::parse_long_arg: Got invalid literal `{}`", rest); - let used: Vec = matcher - .arg_names() - .filter(|&n| { - self.app.find(n).map_or(true, |a| { - !(a.is_set(ArgSettings::Hidden) - || self.required.contains(&a.id)) - }) - }) - .cloned() - .collect(); - - Err(ClapError::invalid_value( - rest.into(), - &["=true", "=false"], - opt, - Usage::new(self).create_usage_no_title(&used), - self.app.color(), - )) - } - }; } + debug!("Parser::parse_long_arg: Found a flag"); + self.check_for_help_and_version_str(&arg)?; + return if let Some(rest) = val { + debug!( + "Parser::parse_long_arg: Got invalid literal `{:?}`", + rest.to_os_string() + ); + let used: Vec = matcher + .arg_names() + .filter(|&n| { + self.app.find(n).map_or(true, |a| { + !(a.is_set(ArgSettings::Hidden) || self.required.contains(&a.id)) + }) + }) + .cloned() + .collect(); + + Err(ClapError::too_many_values( + rest.to_string_lossy().into(), + opt, + Usage::new(self).create_usage_no_title(&used), + self.app.color(), + )) + } else { + debug!("Parser::parse_long_arg: Presence validated"); + Ok(self.parse_flag(opt, matcher)) + }; } if let Some(sc_name) = self.possible_long_flag_subcommand(&arg) { @@ -1448,13 +1439,6 @@ impl<'help, 'app> Parser<'help, 'app> { ParseResult::Flag } - fn parse_flag_false(&self, flag: &Arg<'help>, matcher: &mut ArgMatcher) -> ParseResult { - debug!("Parser::parse_flag_false"); - // This should reset the number of occurrences. - matcher.remove(&flag.id); - ParseResult::Flag - } - fn remove_overrides(&mut self, matcher: &mut ArgMatcher) { debug!("Parser::remove_overrides"); let mut to_rem: Vec = Vec::new(); diff --git a/tests/flags.rs b/tests/flags.rs index a49429ef427..f29ad7a4629 100644 --- a/tests/flags.rs +++ b/tests/flags.rs @@ -87,30 +87,22 @@ fn flag_using_long() { assert!(m.is_present("color")); } +#[cfg(feature = "suggestions")] #[test] fn flag_using_long_with_literals() { + use clap::ErrorKind; + let m = App::new("flag") - .args(&[ - Arg::from("--flag 'some flag'"), - Arg::from("--color 'some other flag'"), - Arg::from("--sky 'a third flag'"), - Arg::from("--rainbow 'yet another flag'").multiple_occurrences(true), - ]) - .get_matches_from(vec![ + .args(&[Arg::from("--rainbow 'yet another flag'").multiple_occurrences(true)]) + .try_get_matches_from(vec![ "", - "--flag=true", - "--color=false", - "--sky", "--rainbow", - "--rainbow=false", // Clears the previous occurrence. "--rainbow", + "--rainbow=false", "--rainbow=true", ]); - assert!(m.is_present("flag")); - assert!(!m.is_present("color")); - assert!(m.is_present("sky")); - assert!(m.is_present("rainbow")); - assert_eq!(m.occurrences_of("rainbow"), 2); + assert!(m.is_err(), "{:#?}", m.unwrap()); + assert_eq!(m.unwrap_err().kind, ErrorKind::TooManyValues); } #[test] From 29e3dc2c47faabd82d5d7b928623963111b29496 Mon Sep 17 00:00:00 2001 From: rami3l Date: Fri, 30 Jul 2021 22:12:56 +0200 Subject: [PATCH 03/11] fix(parser): run `check_for_help_and_version_str` only if no literal is found with flag --- src/parse/parser.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 72ac26d313f..6d5fa9b4655 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -1082,8 +1082,7 @@ impl<'help, 'app> Parser<'help, 'app> { return self.parse_opt(&val, opt, matcher); } debug!("Parser::parse_long_arg: Found a flag"); - self.check_for_help_and_version_str(&arg)?; - return if let Some(rest) = val { + if let Some(rest) = val { debug!( "Parser::parse_long_arg: Got invalid literal `{:?}`", rest.to_os_string() @@ -1098,16 +1097,16 @@ impl<'help, 'app> Parser<'help, 'app> { .cloned() .collect(); - Err(ClapError::too_many_values( + return Err(ClapError::too_many_values( rest.to_string_lossy().into(), opt, Usage::new(self).create_usage_no_title(&used), self.app.color(), - )) - } else { - debug!("Parser::parse_long_arg: Presence validated"); - Ok(self.parse_flag(opt, matcher)) - }; + )); + } + self.check_for_help_and_version_str(&arg)?; + debug!("Parser::parse_long_arg: Presence validated"); + return Ok(self.parse_flag(opt, matcher)); } if let Some(sc_name) = self.possible_long_flag_subcommand(&arg) { From a4a4d4c30769e837a186ea26503a182eb49ff2f3 Mon Sep 17 00:00:00 2001 From: rami3l Date: Fri, 30 Jul 2021 22:28:15 +0200 Subject: [PATCH 04/11] fix(style): fix code format warning --- src/build/app/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs index 167aa1cfb64..5ac56422db1 100644 --- a/src/build/app/mod.rs +++ b/src/build/app/mod.rs @@ -2485,8 +2485,7 @@ impl<'help> App<'help> { { debug!("App::_check_help_and_version: Building help subcommand"); self.subcommands.push( - App::new("help") - .about("Print this message or the help of the given subcommand(s)"), + App::new("help").about("Print this message or the help of the given subcommand(s)"), ); } } From 38649cc0b5c74b0c49b1c7d0ed2b4bde3bfbf1f1 Mon Sep 17 00:00:00 2001 From: rami3l Date: Fri, 30 Jul 2021 22:44:52 +0200 Subject: [PATCH 05/11] docs: update example, mentioning the `pseudo-flag` pattern --- examples/05_flag_args.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/examples/05_flag_args.rs b/examples/05_flag_args.rs index 6dfc193b43e..c0d34e23819 100644 --- a/examples/05_flag_args.rs +++ b/examples/05_flag_args.rs @@ -29,6 +29,15 @@ fn main() { // also has a conflicts_with_all(Vec<&str>) // and an exclusive(true) ) + .arg( + // Sometimes we might want to accept one of the following: `--pseudo-flag`, `--pseudo-flag=true`, `--pseudo-flag=false.` + // The following is the `pseudo-flag` pattern stated in https://github.com/clap-rs/clap/issues/1649#issuecomment-661274943 + Arg::new("pseudo-flag") // Create a "pesudo-flag" with optional value + .possible_values(&["true", "false"]) // Limit that value to `true` of `false` + .long("pseudo-flag") + .min_values(0) + .max_values(1), + ) .arg("-c, --config=[FILE] 'sets a custom config file'") .arg(" 'sets an output file'") .get_matches(); @@ -38,6 +47,11 @@ fn main() { println!("Awesomeness is turned on"); } + // Same thing with `pseudo-flag` + if app.value_of("pseudo-flag") != Some("false") { + println!("Pseudo-flag is turned on"); + } + // If we set the multiple option of a flag we can check how many times the user specified // // Note: if we did not specify the multiple option, and the user used "awesome" we would get From 9eaae979e49c88c76b892085b7ecb39b4bb51a8d Mon Sep 17 00:00:00 2001 From: rami3l Date: Fri, 30 Jul 2021 22:51:55 +0200 Subject: [PATCH 06/11] docs: fix typo in example --- examples/05_flag_args.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/05_flag_args.rs b/examples/05_flag_args.rs index c0d34e23819..d156fb30af9 100644 --- a/examples/05_flag_args.rs +++ b/examples/05_flag_args.rs @@ -48,7 +48,7 @@ fn main() { } // Same thing with `pseudo-flag` - if app.value_of("pseudo-flag") != Some("false") { + if matches.value_of("pseudo-flag") != Some("false") { println!("Pseudo-flag is turned on"); } From 7f8f5ccc9a4361b974a023c0c3c922310146abc7 Mon Sep 17 00:00:00 2001 From: rami3l Date: Sun, 1 Aug 2021 11:41:29 +0200 Subject: [PATCH 07/11] Revert "fix(style): fix code format warning" This reverts commit a4a4d4c30769e837a186ea26503a182eb49ff2f3. --- src/build/app/mod.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs index 5ac56422db1..167aa1cfb64 100644 --- a/src/build/app/mod.rs +++ b/src/build/app/mod.rs @@ -2485,7 +2485,8 @@ impl<'help> App<'help> { { debug!("App::_check_help_and_version: Building help subcommand"); self.subcommands.push( - App::new("help").about("Print this message or the help of the given subcommand(s)"), + App::new("help") + .about("Print this message or the help of the given subcommand(s)"), ); } } From cf94a5bffa9982871e4cc8b15f39525f59828a4b Mon Sep 17 00:00:00 2001 From: rami3l Date: Sun, 1 Aug 2021 11:49:37 +0200 Subject: [PATCH 08/11] docs(example): update `pseudo-flag` example --- examples/05_flag_args.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/examples/05_flag_args.rs b/examples/05_flag_args.rs index d156fb30af9..84ab6ae392a 100644 --- a/examples/05_flag_args.rs +++ b/examples/05_flag_args.rs @@ -33,10 +33,7 @@ fn main() { // Sometimes we might want to accept one of the following: `--pseudo-flag`, `--pseudo-flag=true`, `--pseudo-flag=false.` // The following is the `pseudo-flag` pattern stated in https://github.com/clap-rs/clap/issues/1649#issuecomment-661274943 Arg::new("pseudo-flag") // Create a "pesudo-flag" with optional value - .possible_values(&["true", "false"]) // Limit that value to `true` of `false` - .long("pseudo-flag") - .min_values(0) - .max_values(1), + .possible_values(&["true", "false"]), // Limit that value to `true` of `false` ) .arg("-c, --config=[FILE] 'sets a custom config file'") .arg(" 'sets an output file'") From 2b3fc4d0b6a7df07ca9d17f9ed56f10c3107ab4b Mon Sep 17 00:00:00 2001 From: rami3l Date: Sun, 1 Aug 2021 11:50:41 +0200 Subject: [PATCH 09/11] test: simplify `flag_using_long_with_literals` --- tests/flags.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/tests/flags.rs b/tests/flags.rs index f29ad7a4629..b91a4b7c554 100644 --- a/tests/flags.rs +++ b/tests/flags.rs @@ -87,20 +87,13 @@ fn flag_using_long() { assert!(m.is_present("color")); } -#[cfg(feature = "suggestions")] #[test] fn flag_using_long_with_literals() { use clap::ErrorKind; let m = App::new("flag") - .args(&[Arg::from("--rainbow 'yet another flag'").multiple_occurrences(true)]) - .try_get_matches_from(vec![ - "", - "--rainbow", - "--rainbow", - "--rainbow=false", - "--rainbow=true", - ]); + .arg(Arg::new("rainbow")) + .try_get_matches_from(vec!["", "--rainbow=false"]); assert!(m.is_err(), "{:#?}", m.unwrap()); assert_eq!(m.unwrap_err().kind, ErrorKind::TooManyValues); } From 97999c2d9678789fe7254b064aa43b19772bb070 Mon Sep 17 00:00:00 2001 From: rami3l Date: Sun, 1 Aug 2021 11:59:59 +0200 Subject: [PATCH 10/11] docs(example): remove `pseudo-flag` example --- examples/05_flag_args.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/examples/05_flag_args.rs b/examples/05_flag_args.rs index 84ab6ae392a..6dfc193b43e 100644 --- a/examples/05_flag_args.rs +++ b/examples/05_flag_args.rs @@ -29,12 +29,6 @@ fn main() { // also has a conflicts_with_all(Vec<&str>) // and an exclusive(true) ) - .arg( - // Sometimes we might want to accept one of the following: `--pseudo-flag`, `--pseudo-flag=true`, `--pseudo-flag=false.` - // The following is the `pseudo-flag` pattern stated in https://github.com/clap-rs/clap/issues/1649#issuecomment-661274943 - Arg::new("pseudo-flag") // Create a "pesudo-flag" with optional value - .possible_values(&["true", "false"]), // Limit that value to `true` of `false` - ) .arg("-c, --config=[FILE] 'sets a custom config file'") .arg(" 'sets an output file'") .get_matches(); @@ -44,11 +38,6 @@ fn main() { println!("Awesomeness is turned on"); } - // Same thing with `pseudo-flag` - if matches.value_of("pseudo-flag") != Some("false") { - println!("Pseudo-flag is turned on"); - } - // If we set the multiple option of a flag we can check how many times the user specified // // Note: if we did not specify the multiple option, and the user used "awesome" we would get From d848c887737abd0773f9ba41af3eba39e0f55525 Mon Sep 17 00:00:00 2001 From: rami3l Date: Sun, 1 Aug 2021 12:19:00 +0200 Subject: [PATCH 11/11] fix(test): fix typo in `flag_using_long_with_literals` --- tests/flags.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/flags.rs b/tests/flags.rs index b91a4b7c554..a3e561de9f9 100644 --- a/tests/flags.rs +++ b/tests/flags.rs @@ -92,7 +92,7 @@ fn flag_using_long_with_literals() { use clap::ErrorKind; let m = App::new("flag") - .arg(Arg::new("rainbow")) + .arg(Arg::new("rainbow").long("rainbow")) .try_get_matches_from(vec!["", "--rainbow=false"]); assert!(m.is_err(), "{:#?}", m.unwrap()); assert_eq!(m.unwrap_err().kind, ErrorKind::TooManyValues);