From d56d8dd59ef0f50b019871012caf6e3bcd395464 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 3 Jun 2022 11:01:53 -0500 Subject: [PATCH 01/11] chore: Make it easy to reproduce CI docs run --- .github/workflows/ci.yml | 2 +- Makefile | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e9ce379c37e..d6bfc8af2e2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -144,7 +144,7 @@ jobs: - name: Check documentation env: RUSTDOCFLAGS: -D warnings - run: cargo doc --workspace --all-features --no-deps --document-private-items + run: make doc rustfmt: name: rustfmt runs-on: ubuntu-latest diff --git a/Makefile b/Makefile index 1f3db6eeb4b..7ac4d449c81 100644 --- a/Makefile +++ b/Makefile @@ -35,3 +35,6 @@ clippy-%: test-ui-%: cargo +${MSRV} test --test derive_ui --features derive ${_FEATURES_${@:test-ui-%=%}} + +doc: + cargo doc --workspace --all-features --no-deps --document-private-items From 52e2874c030ed680d0f831b62af8731f89c75c9c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 3 Jun 2022 12:11:45 -0500 Subject: [PATCH 02/11] fix(builder): Make it easier to discover/access ArgAction --- src/builder/action.rs | 20 ++++++++++---------- src/builder/arg.rs | 4 ++-- src/lib.rs | 1 + src/parser/parser.rs | 2 +- tests/builder/action.rs | 2 +- 5 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/builder/action.rs b/src/builder/action.rs index fa1ccd57eef..acfe2027b12 100644 --- a/src/builder/action.rs +++ b/src/builder/action.rs @@ -9,7 +9,7 @@ /// .arg( /// Arg::new("special-help") /// .short('?') -/// .action(clap::builder::ArgAction::Help) +/// .action(clap::ArgAction::Help) /// ); /// /// // Existing help still exists @@ -35,7 +35,7 @@ pub enum ArgAction { /// .arg( /// Arg::new("flag") /// .long("flag") - /// .action(clap::builder::ArgAction::Set) + /// .action(clap::ArgAction::Set) /// ); /// /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "value"]).unwrap(); @@ -59,7 +59,7 @@ pub enum ArgAction { /// Arg::new("flag") /// .long("flag") /// .multiple_occurrences(true) - /// .action(clap::builder::ArgAction::Append) + /// .action(clap::ArgAction::Append) /// ); /// /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "value1", "--flag", "value2"]).unwrap(); @@ -82,7 +82,7 @@ pub enum ArgAction { /// .arg( /// Arg::new("flag") /// .long("flag") - /// .action(clap::builder::ArgAction::StoreValue) + /// .action(clap::ArgAction::StoreValue) /// ); /// /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "value"]).unwrap(); @@ -108,7 +108,7 @@ pub enum ArgAction { /// Arg::new("flag") /// .long("flag") /// .multiple_occurrences(true) - /// .action(clap::builder::ArgAction::IncOccurrence) + /// .action(clap::ArgAction::IncOccurrence) /// ); /// /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); @@ -132,7 +132,7 @@ pub enum ArgAction { /// .arg( /// Arg::new("flag") /// .long("flag") - /// .action(clap::builder::ArgAction::SetTrue) + /// .action(clap::ArgAction::SetTrue) /// ); /// /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); @@ -167,7 +167,7 @@ pub enum ArgAction { /// .arg( /// Arg::new("flag") /// .long("flag") - /// .action(clap::builder::ArgAction::SetFalse) + /// .action(clap::ArgAction::SetFalse) /// ); /// /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); @@ -202,7 +202,7 @@ pub enum ArgAction { /// .arg( /// Arg::new("flag") /// .long("flag") - /// .action(clap::builder::ArgAction::Count) + /// .action(clap::ArgAction::Count) /// ); /// /// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap(); @@ -235,7 +235,7 @@ pub enum ArgAction { /// .arg( /// Arg::new("special-help") /// .short('?') - /// .action(clap::builder::ArgAction::Help) + /// .action(clap::ArgAction::Help) /// ); /// /// // Existing help still exists @@ -261,7 +261,7 @@ pub enum ArgAction { /// .arg( /// Arg::new("special-version") /// .long("special-version") - /// .action(clap::builder::ArgAction::Version) + /// .action(clap::ArgAction::Version) /// ); /// /// // Existing help still exists diff --git a/src/builder/arg.rs b/src/builder/arg.rs index 6f9fc767887..6ac182719fd 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -18,9 +18,9 @@ use yaml_rust::Yaml; // Internal use crate::builder::usage_parser::UsageParser; -use crate::builder::ArgAction; use crate::builder::ArgPredicate; use crate::util::{Id, Key}; +use crate::ArgAction; use crate::PossibleValue; use crate::ValueHint; use crate::INTERNAL_ERROR_MSG; @@ -1014,7 +1014,7 @@ impl<'help> Arg<'help> { /// .arg( /// Arg::new("flag") /// .long("flag") - /// .action(clap::builder::ArgAction::StoreValue) + /// .action(clap::ArgAction::StoreValue) /// ); /// /// let matches = cmd.try_get_matches_from(["mycmd", "--flag", "value"]).unwrap(); diff --git a/src/lib.rs b/src/lib.rs index 435539b14a9..f1f58a9712d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -26,6 +26,7 @@ #[cfg(not(feature = "std"))] compile_error!("`std` feature is currently required to build `clap`"); +pub use crate::builder::ArgAction; pub use crate::builder::Command; pub use crate::builder::{Arg, ArgGroup}; pub use crate::error::Error; diff --git a/src/parser/parser.rs b/src/parser/parser.rs index 4a695c26638..cb54422342b 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -9,7 +9,6 @@ use clap_lex::RawOsStr; // Internal use crate::builder::AppSettings as AS; -use crate::builder::ArgAction; use crate::builder::{Arg, Command}; use crate::error::Error as ClapError; use crate::error::Result as ClapResult; @@ -20,6 +19,7 @@ use crate::parser::features::suggestions; use crate::parser::{ArgMatcher, SubCommand}; use crate::parser::{Validator, ValueSource}; use crate::util::Id; +use crate::ArgAction; use crate::{INTERNAL_ERROR_MSG, INVALID_UTF8}; pub(crate) struct Parser<'help, 'cmd> { diff --git a/tests/builder/action.rs b/tests/builder/action.rs index 1697914d84a..a6f73d2471e 100644 --- a/tests/builder/action.rs +++ b/tests/builder/action.rs @@ -1,7 +1,7 @@ #![allow(clippy::bool_assert_comparison)] -use clap::builder::ArgAction; use clap::Arg; +use clap::ArgAction; use clap::Command; #[test] From 11fbe7e54bc4a9247099d3928f67195a2ed36986 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 3 Jun 2022 13:25:17 -0500 Subject: [PATCH 03/11] test(derive): Improve help output assertions --- tests/derive/help.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/derive/help.rs b/tests/derive/help.rs index 1edf42d9193..2fdae51f6b1 100644 --- a/tests/derive/help.rs +++ b/tests/derive/help.rs @@ -363,7 +363,7 @@ OPTIONS: let mut buffer: Vec = Default::default(); cmd.write_help(&mut buffer).unwrap(); let help = String::from_utf8(buffer).unwrap(); - assert_eq!(help, HELP); + snapbox::assert_eq(HELP, help); } #[test] @@ -420,7 +420,7 @@ OPTIONS: let mut buffer: Vec = Default::default(); cmd.write_help(&mut buffer).unwrap(); let help = String::from_utf8(buffer).unwrap(); - assert_eq!(help, HELP); + snapbox::assert_eq(HELP, help); } #[test] @@ -476,5 +476,5 @@ OPTIONS: let mut buffer: Vec = Default::default(); cmd.write_help(&mut buffer).unwrap(); let help = String::from_utf8(buffer).unwrap(); - assert_eq!(help, HELP); + snapbox::assert_eq(HELP, help); } From a3092bafcee702cf5672ef53e17440f252ca6a37 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Fri, 3 Jun 2022 13:28:08 -0500 Subject: [PATCH 04/11] fix(help): Don't report flag defaults Now that flags can have meaningful defaults and with defaults being implicitly set for certain actions, they appear in help but don't quite make sense. --- src/output/help.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/output/help.rs b/src/output/help.rs index 04e0a7a2ec0..152955d25a8 100644 --- a/src/output/help.rs +++ b/src/output/help.rs @@ -601,7 +601,7 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> { spec_vals.push(env_info); } } - if !a.is_hide_default_value_set() && !a.default_vals.is_empty() { + if a.is_takes_value_set() && !a.is_hide_default_value_set() && !a.default_vals.is_empty() { debug!( "Help::spec_vals: Found default value...[{:?}]", a.default_vals From b09def1ad0daffdb1dc4eeea63a840d9a5a2dc4e Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 4 Jun 2022 07:02:20 -0500 Subject: [PATCH 05/11] test(derive): Verify doc comments on ArgEnum --- tests/derive/help.rs | 47 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/tests/derive/help.rs b/tests/derive/help.rs index 2fdae51f6b1..4254dda5a01 100644 --- a/tests/derive/help.rs +++ b/tests/derive/help.rs @@ -478,3 +478,50 @@ OPTIONS: let help = String::from_utf8(buffer).unwrap(); snapbox::assert_eq(HELP, help); } + +#[test] +#[cfg(feature = "unstable-v4")] +fn derive_possible_value_help() { + static HELP: &str = "clap +Application help + +USAGE: + clap + +ARGS: + + Argument help + + Possible values: + - foo: Foo help + - bar: Bar help + +OPTIONS: + -h, --help + Print help information +"; + + /// Application help + #[derive(Parser, PartialEq, Debug)] + struct Args { + /// Argument help + #[clap(arg_enum, value_parser)] + arg: ArgChoice, + } + + #[derive(clap::ArgEnum, PartialEq, Debug, Clone)] + enum ArgChoice { + /// Foo help + Foo, + /// Bar help + Bar, + } + + use clap::CommandFactory; + let mut cmd = Args::command(); + + let mut buffer: Vec = Default::default(); + cmd.write_long_help(&mut buffer).unwrap(); + let help = String::from_utf8(buffer).unwrap(); + snapbox::assert_eq(HELP, help); +} From ccd6663de9a6b2b8d3c2a15c308b417de2fd077b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 4 Jun 2022 08:01:36 -0500 Subject: [PATCH 06/11] fix(help): Output some bool possible values Originally I hid all, assuming the flag-only use case but we had to prevent that from showing up anyways. For the takes_value case, we should be showing something since we know what it accepts. I decided to only show the most basic values and hide the rest so as to not overwhelm the user with redundant options and hope the user recognizes they are redundant. --- src/builder/value_parser.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/builder/value_parser.rs b/src/builder/value_parser.rs index 01aeef57f6f..30b3610d0aa 100644 --- a/src/builder/value_parser.rs +++ b/src/builder/value_parser.rs @@ -1252,7 +1252,7 @@ impl BoolValueParser { ["true", "false"] .iter() .copied() - .map(|l| crate::PossibleValue::new(l).hide(true)) + .map(|l| crate::PossibleValue::new(l)) } } @@ -1352,7 +1352,7 @@ impl FalseyValueParser { .iter() .chain(crate::util::FALSE_LITERALS.iter()) .copied() - .map(|l| crate::PossibleValue::new(l).hide(true)) + .map(|l| crate::PossibleValue::new(l).hide(l != "true" && l != "false")) } } @@ -1449,7 +1449,7 @@ impl BoolishValueParser { .iter() .chain(crate::util::FALSE_LITERALS.iter()) .copied() - .map(|l| crate::PossibleValue::new(l).hide(true)) + .map(|l| crate::PossibleValue::new(l).hide(l != "true" && l != "false")) } } From 5f56e93c0f5b58b7fe0e409fc200c1ec22833382 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 4 Jun 2022 08:13:03 -0500 Subject: [PATCH 07/11] refactor(parser): Clarify we are working with ids, not names --- src/parser/arg_matcher.rs | 2 +- src/parser/parser.rs | 6 +++--- src/parser/validator.rs | 20 ++++++++++---------- 3 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index eab4999189f..690f504b8cc 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -116,7 +116,7 @@ impl ArgMatcher { self.matches.args.contains_key(arg) } - pub(crate) fn arg_names(&self) -> indexmap::map::Keys { + pub(crate) fn arg_ids(&self) -> indexmap::map::Keys { self.matches.args.keys() } diff --git a/src/parser/parser.rs b/src/parser/parser.rs index cb54422342b..d3db5c2cf9a 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -771,7 +771,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { long_arg, rest ); let used: Vec = matcher - .arg_names() + .arg_ids() .filter(|&n| { self.cmd .find(n) @@ -1312,7 +1312,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // Override anything that can override us let mut transitive = Vec::new(); - for arg_id in matcher.arg_names() { + for arg_id in matcher.arg_ids() { if let Some(overrider) = self.cmd.find(arg_id) { if overrider.overrides.contains(&arg.id) { transitive.push(&overrider.id); @@ -1612,7 +1612,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { let required = self.cmd.required_graph(); let used: Vec = matcher - .arg_names() + .arg_ids() .filter(|n| self.cmd.find(n).map_or(true, |a| !a.is_hide_set())) .cloned() .collect(); diff --git a/src/parser/validator.rs b/src/parser/validator.rs index dca9789e572..cc74daba590 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -52,7 +52,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { if !has_subcmd && self.cmd.is_arg_required_else_help_set() { let num_user_values = matcher - .arg_names() + .arg_ids() .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) .count(); if num_user_values == 0 { @@ -179,7 +179,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { self.validate_exclusive(matcher)?; for arg_id in matcher - .arg_names() + .arg_ids() .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) .filter(|arg_id| self.cmd.find(arg_id).is_some()) { @@ -194,14 +194,14 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { fn validate_exclusive(&self, matcher: &ArgMatcher) -> ClapResult<()> { debug!("Validator::validate_exclusive"); // Not bothering to filter for `check_explicit` since defaults shouldn't play into this - let args_count = matcher.arg_names().count(); + let args_count = matcher.arg_ids().count(); if args_count <= 1 { // Nothing present to conflict with return Ok(()); } matcher - .arg_names() + .arg_ids() .filter_map(|name| { debug!("Validator::validate_exclusive:iter:{:?}", name); self.cmd @@ -263,7 +263,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { fn build_conflict_err_usage(&self, matcher: &ArgMatcher, conflicting_keys: &[Id]) -> String { let used_filtered: Vec = matcher - .arg_names() + .arg_ids() .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) .filter(|n| { // Filter out the args we don't want to specify. @@ -288,7 +288,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { fn gather_requires(&mut self, matcher: &ArgMatcher) { debug!("Validator::gather_requires"); for name in matcher - .arg_names() + .arg_ids() .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) { debug!("Validator::gather_requires:iter:{:?}", name); @@ -455,9 +455,9 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { debug!("Validator::validate_required: required={:?}", self.required); self.gather_requires(matcher); - let is_exclusive_present = matcher.arg_names().any(|name| { + let is_exclusive_present = matcher.arg_ids().any(|id| { self.cmd - .find(name) + .find(id) .map(|arg| arg.is_exclusive_set()) .unwrap_or_default() }); @@ -569,7 +569,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { ); let used: Vec = matcher - .arg_names() + .arg_ids() .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) .filter(|n| { // Filter out the args we don't want to specify. @@ -601,7 +601,7 @@ impl Conflicts { debug!("Conflicts::gather_conflicts: arg={:?}", arg_id); let mut conflicts = Vec::new(); for other_arg_id in matcher - .arg_names() + .arg_ids() .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) { if arg_id == other_arg_id { From a97134600499fdb86ceb587876a3be6638add9dd Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 4 Jun 2022 08:27:39 -0500 Subject: [PATCH 08/11] test(parser): Verify indices of defaults I thought I had broken this but it always seemed to have worked this way --- tests/builder/default_missing_vals.rs | 33 +++++++++++++++++++++++++++ tests/builder/default_vals.rs | 14 ++++++++++++ 2 files changed, 47 insertions(+) diff --git a/tests/builder/default_missing_vals.rs b/tests/builder/default_missing_vals.rs index bacbe1ff8e6..1d2913e4d12 100644 --- a/tests/builder/default_missing_vals.rs +++ b/tests/builder/default_missing_vals.rs @@ -24,6 +24,7 @@ fn opt_missing() { m.value_source("color").unwrap(), clap::ValueSource::DefaultValue ); + assert_eq!(m.index_of("color"), Some(1)); } #[test] @@ -50,6 +51,7 @@ fn opt_present_with_missing_value() { m.value_source("color").unwrap(), clap::ValueSource::CommandLine ); + assert_eq!(m.index_of("color"), Some(2)); } #[test] @@ -76,6 +78,7 @@ fn opt_present_with_value() { m.value_source("color").unwrap(), clap::ValueSource::CommandLine ); + assert_eq!(m.index_of("color"), Some(2)); } #[test] @@ -101,6 +104,7 @@ fn opt_present_with_empty_value() { m.value_source("color").unwrap(), clap::ValueSource::CommandLine ); + assert_eq!(m.index_of("color"), Some(2)); } //## `default_value`/`default_missing_value` non-interaction checks @@ -271,3 +275,32 @@ fn default_missing_values_are_valid() { ) .try_get_matches(); } + +#[test] +fn valid_index() { + let m = Command::new("df") + .arg( + Arg::new("color") + .long("color") + .default_value("auto") + .min_values(0) + .require_equals(true) + .default_missing_value("always"), + ) + .arg(Arg::new("sync").long("sync")) + .try_get_matches_from(vec!["df", "--color", "--sync"]) + .unwrap(); + assert!(m.is_present("color")); + assert_eq!( + m.get_one::("color").map(|v| v.as_str()).unwrap(), + "always" + ); + assert_eq!(m.occurrences_of("color"), 1); + assert_eq!( + m.value_source("color").unwrap(), + clap::ValueSource::CommandLine + ); + + // Make sure the index reflects `--color`s position and not something else + assert_eq!(m.index_of("color"), Some(2)); +} diff --git a/tests/builder/default_vals.rs b/tests/builder/default_vals.rs index 501ffcd386a..b6de1e52e1c 100644 --- a/tests/builder/default_vals.rs +++ b/tests/builder/default_vals.rs @@ -19,6 +19,20 @@ fn opts() { ); } +#[test] +fn default_has_index() { + let r = Command::new("df") + .arg( + arg!(o: -o "some opt") + .required(false) + .default_value("default"), + ) + .try_get_matches_from(vec![""]); + assert!(r.is_ok(), "{}", r.unwrap_err()); + let m = r.unwrap(); + assert_eq!(m.index_of("o"), Some(1)); +} + #[test] fn opt_without_value_fail() { let r = Command::new("df") From f3bc3d5eb71ac22230bfdbdeba16dc6d05ee49c8 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 4 Jun 2022 08:41:34 -0500 Subject: [PATCH 09/11] fix(error): Don't include default-but-required arguments in usage --- src/parser/parser.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/parser/parser.rs b/src/parser/parser.rs index d3db5c2cf9a..e6d9980081b 100644 --- a/src/parser/parser.rs +++ b/src/parser/parser.rs @@ -772,6 +772,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { ); let used: Vec = matcher .arg_ids() + .filter(|arg_id| { + matcher.check_explicit(arg_id, crate::builder::ArgPredicate::IsPresent) + }) .filter(|&n| { self.cmd .find(n) @@ -1613,6 +1616,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { let required = self.cmd.required_graph(); let used: Vec = matcher .arg_ids() + .filter(|arg_id| { + matcher.check_explicit(arg_id, crate::builder::ArgPredicate::IsPresent) + }) .filter(|n| self.cmd.find(n).map_or(true, |a| !a.is_hide_set())) .cloned() .collect(); From 9638f33d2fbf752f1332cf2e7b6ab0febb73ad18 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 4 Jun 2022 08:53:10 -0500 Subject: [PATCH 10/11] fix(parser): Exclusive shouldnt preclude defaults Unsure why a comment said this doesn't matter. It matters both for counting arguments and for reporting the correct argument is exclusive. --- src/parser/validator.rs | 26 +++++++++++++------ tests/builder/conflicts.rs | 52 +++++++++++++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 9 deletions(-) diff --git a/src/parser/validator.rs b/src/parser/validator.rs index cc74daba590..dc4c3598fd1 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -193,8 +193,12 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { fn validate_exclusive(&self, matcher: &ArgMatcher) -> ClapResult<()> { debug!("Validator::validate_exclusive"); - // Not bothering to filter for `check_explicit` since defaults shouldn't play into this - let args_count = matcher.arg_ids().count(); + let args_count = matcher + .arg_ids() + .filter(|arg_id| { + matcher.check_explicit(arg_id, crate::builder::ArgPredicate::IsPresent) + }) + .count(); if args_count <= 1 { // Nothing present to conflict with return Ok(()); @@ -202,6 +206,9 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { matcher .arg_ids() + .filter(|arg_id| { + matcher.check_explicit(arg_id, crate::builder::ArgPredicate::IsPresent) + }) .filter_map(|name| { debug!("Validator::validate_exclusive:iter:{:?}", name); self.cmd @@ -455,12 +462,15 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { debug!("Validator::validate_required: required={:?}", self.required); self.gather_requires(matcher); - let is_exclusive_present = matcher.arg_ids().any(|id| { - self.cmd - .find(id) - .map(|arg| arg.is_exclusive_set()) - .unwrap_or_default() - }); + let is_exclusive_present = matcher + .arg_ids() + .filter(|arg_id| matcher.check_explicit(arg_id, ArgPredicate::IsPresent)) + .any(|id| { + self.cmd + .find(id) + .map(|arg| arg.is_exclusive_set()) + .unwrap_or_default() + }); debug!( "Validator::validate_required: is_exclusive_present={}", is_exclusive_present diff --git a/tests/builder/conflicts.rs b/tests/builder/conflicts.rs index 80d2d31f727..345afa50674 100644 --- a/tests/builder/conflicts.rs +++ b/tests/builder/conflicts.rs @@ -62,7 +62,7 @@ fn flag_conflict_with_all() { } #[test] -fn flag_conflict_with_everything() { +fn exclusive_flag() { let result = Command::new("flag_conflict") .arg(arg!(-f --flag "some flag").exclusive(true)) .arg(arg!(-o --other "some flag")) @@ -72,6 +72,56 @@ fn flag_conflict_with_everything() { assert_eq!(err.kind(), ErrorKind::ArgumentConflict); } +#[test] +fn exclusive_option() { + let result = Command::new("flag_conflict") + .arg( + arg!(-f --flag "some flag") + .required(false) + .exclusive(true), + ) + .arg(arg!(-o --other "some flag").required(false)) + .try_get_matches_from(vec!["myprog", "-o=val1", "-f=val2"]); + assert!(result.is_err()); + let err = result.err().unwrap(); + assert_eq!(err.kind(), ErrorKind::ArgumentConflict); +} + +#[test] +fn not_exclusive_with_defaults() { + let result = Command::new("flag_conflict") + .arg( + arg!(-f --flag "some flag") + .required(false) + .exclusive(true), + ) + .arg( + arg!(-o --other "some flag") + .required(false) + .default_value("val1"), + ) + .try_get_matches_from(vec!["myprog", "-f=val2"]); + assert!(result.is_ok(), "{}", result.unwrap_err()); +} + +#[test] +fn default_doesnt_activate_exclusive() { + let result = Command::new("flag_conflict") + .arg( + arg!(-f --flag "some flag") + .required(false) + .exclusive(true) + .default_value("val2"), + ) + .arg( + arg!(-o --other "some flag") + .required(false) + .default_value("val1"), + ) + .try_get_matches_from(vec!["myprog"]); + assert!(result.is_ok(), "{}", result.unwrap_err()); +} + #[test] fn arg_conflicts_with_group() { let mut cmd = Command::new("group_conflict") From 002204a122f4b89d09512114a944d80f8ed2c9b3 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Sat, 4 Jun 2022 13:24:46 -0500 Subject: [PATCH 11/11] test(derive): Update ui tests --- tests/derive_ui/next/tuple_struct.stderr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/derive_ui/next/tuple_struct.stderr b/tests/derive_ui/next/tuple_struct.stderr index 3055c31a1b6..bf2bac0634a 100644 --- a/tests/derive_ui/next/tuple_struct.stderr +++ b/tests/derive_ui/next/tuple_struct.stderr @@ -17,6 +17,6 @@ error[E0599]: no function or associated item named `parse` found for struct `Opt | = help: items from traits can only be used if the trait is implemented and in scope = note: the following traits define an item `parse`, perhaps you need to implement one of them: - candidate #1: `Parser` + candidate #1: `StructOpt` candidate #2: `AnyValueParser` candidate #3: `TypedValueParser`