From 3d37001d1dc647d73cc597ff172f1072d4beb80d Mon Sep 17 00:00:00 2001 From: Kevin K Date: Mon, 31 Oct 2016 00:35:23 -0400 Subject: [PATCH] imp(Error Output): conflicting errors are now symetrical, meaning more consistent and less confusing Prior to this commit, conflicting error messages and the suggeseted usage would depend on whether you defined the conflict on both arguments, or just one, and the order in which you specified the conflicting arguments at runtime. Now they are symetrical, meaning the suggestions from the error message are consistent, and it no longer matters if you specify the conflict in one, or both arguments. Closes #718 --- src/app/macros.rs | 88 ++++++++++++++++++++++++- src/app/parser.rs | 131 +++++++++++++++++--------------------- src/args/any_arg.rs | 3 +- src/completions/macros.rs | 2 +- src/macros.rs | 4 +- 5 files changed, 150 insertions(+), 78 deletions(-) diff --git a/src/app/macros.rs b/src/app/macros.rs index d3006b89..69be5bfc 100644 --- a/src/app/macros.rs +++ b/src/app/macros.rs @@ -27,7 +27,7 @@ macro_rules! arg_post_processing { // Handle POSIX overrides debug!("Is '{}' in overrides...", $arg.to_string()); if $me.overrides.contains(&$arg.name()) { - if let Some(ref name) = $me.overriden_from($arg.name(), $matcher) { + if let Some(ref name) = find_name_from!($me, &$arg.name(), overrides, $matcher) { sdebugln!("Yes by {}", name); $matcher.remove(name); remove_overriden!($me, name); @@ -48,6 +48,32 @@ macro_rules! arg_post_processing { debug!("Does '{}' have conflicts...", $arg.to_string()); if let Some(bl) = $arg.blacklist() { sdebugln!("Yes"); + + for c in bl { + // Inject two-way conflicts + debug!("Has '{}' already been matched...", c); + if $matcher.contains(c) { + sdebugln!("Yes"); + // find who blacklisted us... + $me.blacklist.push(&$arg.name); + // if let Some(f) = $me.find_flag_mut(c) { + // if let Some(ref mut bl) = f.blacklist { + // bl.push(&$arg.name); + // } + // } else if let Some(o) = $me.find_option_mut(c) { + // if let Some(ref mut bl) = o.blacklist { + // bl.push(&$arg.name); + // } + // } else if let Some(p) = $me.find_positional_mut(c) { + // if let Some(ref mut bl) = p.blacklist { + // bl.push(&$arg.name); + // } + // } + } else { + sdebugln!("No"); + } + } + $me.blacklist.extend(bl); vec_remove_all!($me.overrides, bl); vec_remove_all!($me.required, bl); @@ -143,3 +169,63 @@ macro_rules! parse_positional { } }; } + +macro_rules! find_from { + ($_self:ident, $arg_name:expr, $from:ident, $matcher:expr) => {{ + let mut ret = None; + for k in $matcher.arg_names() { + if let Some(f) = $_self.find_flag(k) { + if let Some(ref v) = f.$from { + if v.contains($arg_name) { + ret = Some(f.to_string()); + } + } + } + if let Some(o) = $_self.find_option(k) { + if let Some(ref v) = o.$from { + if v.contains(&$arg_name) { + ret = Some(o.to_string()); + } + } + } + if let Some(pos) = $_self.find_positional(k) { + if let Some(ref v) = pos.$from { + if v.contains($arg_name) { + ret = Some(pos.name.to_owned()); + } + } + } + } + ret + }}; +} + +macro_rules! find_name_from { + ($_self:ident, $arg_name:expr, $from:ident, $matcher:expr) => {{ + let mut ret = None; + for k in $matcher.arg_names() { + if let Some(f) = $_self.find_flag(k) { + if let Some(ref v) = f.$from { + if v.contains($arg_name) { + ret = Some(f.name); + } + } + } + if let Some(o) = $_self.find_option(k) { + if let Some(ref v) = o.$from { + if v.contains(&$arg_name) { + ret = Some(o.name); + } + } + } + if let Some(pos) = $_self.find_positional(k) { + if let Some(ref v) = pos.$from { + if v.contains($arg_name) { + ret = Some(pos.name); + } + } + } + } + ret + }}; +} \ No newline at end of file diff --git a/src/app/parser.rs b/src/app/parser.rs index a3b0abfa..8a35d3be 100644 --- a/src/app/parser.rs +++ b/src/app/parser.rs @@ -1007,59 +1007,6 @@ impl<'a, 'b> Parser<'a, 'b> Ok(()) } - fn blacklisted_from(&self, name: &str, matcher: &ArgMatcher) -> Option { - for k in matcher.arg_names() { - if let Some(f) = self.flags.iter().find(|f| &f.name == &k) { - if let Some(ref bl) = f.blacklist { - if bl.contains(&name) { - return Some(f.to_string()); - } - } - } - if let Some(o) = self.opts.iter().find(|o| &o.name == &k) { - if let Some(ref bl) = o.blacklist { - if bl.contains(&name) { - return Some(o.to_string()); - } - } - } - if let Some(pos) = self.positionals.values().find(|p| &p.name == &k) { - if let Some(ref bl) = pos.blacklist { - if bl.contains(&name) { - return Some(pos.name.to_owned()); - } - } - } - } - None - } - - fn overriden_from(&self, name: &str, matcher: &ArgMatcher) -> Option<&'a str> { - for k in matcher.arg_names() { - if let Some(f) = self.flags.iter().find(|f| &f.name == &k) { - if let Some(ref bl) = f.overrides { - if bl.contains(&name.into()) { - return Some(f.name); - } - } - } - if let Some(o) = self.opts.iter().find(|o| &o.name == &k) { - if let Some(ref bl) = o.overrides { - if bl.contains(&name.into()) { - return Some(o.name); - } - } - } - if let Some(pos) = self.positionals.values().find(|p| &p.name == &k) { - if let Some(ref bl) = pos.overrides { - if bl.contains(&name.into()) { - return Some(pos.name); - } - } - } - } - None - } fn groups_for_arg(&self, name: &str) -> Option> { debugln!("fn=groups_for_arg; name={}", name); @@ -1535,26 +1482,43 @@ impl<'a, 'b> Parser<'a, 'b> } fn validate_blacklist(&self, matcher: &mut ArgMatcher) -> ClapResult<()> { - debugln!("fn=validate_blacklist;"); + debugln!("fn=validate_blacklist;blacklist={:?}", self.blacklist); macro_rules! build_err { ($me:ident, $name:expr, $matcher:ident) => ({ - debugln!("macro=build_err;"); - let c_with = $me.blacklisted_from($name, &$matcher); + debugln!("macro=build_err;name={}", $name); + let mut c_with = find_from!($me, $name, blacklist, &$matcher); + c_with = if c_with.is_none() { + if let Some(aa) = $me.find_any_arg($name) { + if let Some(bl) = aa.blacklist() { + if let Some(arg_name) = bl.iter().find(|arg| $matcher.contains(arg)) { + if let Some(aa) = $me.find_any_arg(arg_name) { + Some(aa.to_string()) + } else { + c_with + } + } else { + c_with + } + } else { + c_with + } + } else { + c_with + } + } else { + c_with + }; debugln!("'{:?}' conflicts with '{}'", c_with, $name); $matcher.remove($name); let usg = $me.create_current_usage($matcher); - if let Some(f) = $me.flags.iter().filter(|f| f.name == $name).next() { + if let Some(f) = $me.find_flag($name) { debugln!("It was a flag..."); Error::argument_conflict(f, c_with, &*usg, self.color()) - } else if let Some(o) = $me.opts.iter() - .filter(|o| o.name == $name) - .next() { - debugln!("It was an option..."); + } else if let Some(o) = $me.find_option($name) { + debugln!("It was an option..."); Error::argument_conflict(o, c_with, &*usg, self.color()) } else { - match $me.positionals.values() - .filter(|p| p.name == $name) - .next() { + match $me.find_positional($name) { Some(p) => { debugln!("It was a positional..."); Error::argument_conflict(p, c_with, &*usg, self.color()) @@ -1572,12 +1536,12 @@ impl<'a, 'b> Parser<'a, 'b> debugln!("Checking arg '{}' in group...", n); if matcher.contains(n) { debugln!("matcher contains it..."); - return Err(build_err!(self, n, matcher)); + return Err(build_err!(self, &n, matcher)); } } } else if matcher.contains(name) { debugln!("matcher contains it..."); - return Err(build_err!(self, *name, matcher)); + return Err(build_err!(self, name, matcher)); } } Ok(()) @@ -1940,15 +1904,15 @@ impl<'a, 'b> Parser<'a, 'b> Ok(()) } - pub fn flags(&self) -> Iter { + pub fn flags(&self) -> Iter> { self.flags.iter() } - pub fn opts(&self) -> Iter { + pub fn opts(&self) -> Iter> { self.opts.iter() } - pub fn positionals(&self) -> vec_map::Values { + pub fn positionals(&self) -> vec_map::Values> { self.positionals.values() } @@ -1973,19 +1937,40 @@ impl<'a, 'b> Parser<'a, 'b> } } - pub fn find_arg(&self, arg: &str) -> Option<&AnyArg> { + pub fn find_any_arg(&self, arg: &str) -> Option<&AnyArg> { + if let Some(f) = self.find_flag(arg) { + return Some(f); + } + if let Some(o) = self.find_option(arg) { + return Some(o); + } + if let Some(p) = self.find_positional(arg) { + return Some(p); + } + None + } + + fn find_flag(&self, name: &str) -> Option<&FlagBuilder<'a, 'b>> { for f in self.flags() { - if f.name == arg { + if f.name == name || f.aliases.as_ref().unwrap_or(&vec![("",false)]).iter().any(|&(n,_)| n == name) { return Some(f); } } + None + } + + fn find_option(&self, name: &str) -> Option<&OptBuilder<'a, 'b>> { for o in self.opts() { - if o.name == arg { + if o.name == name || o.aliases.as_ref().unwrap_or(&vec![("",false)]).iter().any(|&(n,_)| n == name) { return Some(o); } } + None + } + + fn find_positional(&self, name: &str) -> Option<&PosBuilder<'a, 'b>> { for p in self.positionals() { - if p.name == arg { + if p.name == name { return Some(p); } } diff --git a/src/args/any_arg.rs b/src/args/any_arg.rs index f15a16ab..09408230 100644 --- a/src/args/any_arg.rs +++ b/src/args/any_arg.rs @@ -1,5 +1,6 @@ // Std use std::rc::Rc; +use std::fmt as std_fmt; // Third Party use vec_map::VecMap; @@ -8,7 +9,7 @@ use vec_map::VecMap; use args::settings::ArgSettings; #[doc(hidden)] -pub trait AnyArg<'n, 'e> { +pub trait AnyArg<'n, 'e>: std_fmt::Display { fn name(&self) -> &'n str; fn overrides(&self) -> Option<&[&'e str]>; fn aliases(&self) -> Option>; diff --git a/src/completions/macros.rs b/src/completions/macros.rs index 0f5f96ed..efb2c093 100644 --- a/src/completions/macros.rs +++ b/src/completions/macros.rs @@ -13,7 +13,7 @@ macro_rules! get_zsh_arg_conflicts { if let Some(conf_vec) = $arg.blacklist() { let mut v = vec![]; for arg_name in conf_vec { - let arg = $p.find_arg(arg_name).expect($msg); + let arg = $p.find_any_arg(arg_name).expect($msg); if let Some(s) = arg.short() { v.push(format!("-{}", s)); } diff --git a/src/macros.rs b/src/macros.rs index 932397f6..9478857c 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -647,7 +647,7 @@ macro_rules! write_nspaces { // convenience macro for remove an item from a vec macro_rules! vec_remove { ($vec:expr, $to_rem:expr) => { - debugln!("macro=vec_remove!;"); + debugln!("macro=vec_remove!;to_rem={:?}", $to_rem); for i in (0 .. $vec.len()).rev() { let should_remove = &$vec[i] == $to_rem; if should_remove { $vec.swap_remove(i); } @@ -658,7 +658,7 @@ macro_rules! vec_remove { // convenience macro for remove an item from a vec macro_rules! vec_remove_all { ($vec:expr, $to_rem:expr) => { - debugln!("macro=vec_remove!;"); + debugln!("macro=vec_remove_all!;to_rem={:?}", $to_rem); for i in (0 .. $vec.len()).rev() { let should_remove = $to_rem.contains(&$vec[i]); if should_remove { $vec.swap_remove(i); }