Skip to content

Commit

Permalink
imp(Error Output): conflicting errors are now symetrical, meaning mor…
Browse files Browse the repository at this point in the history 
…e 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
  • Loading branch information
@kbknapp
kbknapp committed Oct 31, 2016
1 parent 44f6b1e commit 3d37001
Show file tree
Hide file tree
Showing 5 changed files with 150 additions and 78 deletions.
88 changes: 87 additions & 1 deletion src/app/macros.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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
}};
}
131 changes: 58 additions & 73 deletions src/app/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1007,59 +1007,6 @@ impl<'a, 'b> Parser<'a, 'b>
Ok(())
}

fn blacklisted_from(&self, name: &str, matcher: &ArgMatcher) -> Option<String> {
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<Vec<&'a str>> {
debugln!("fn=groups_for_arg; name={}", name);
Expand Down Expand Up @@ -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())
Expand All @@ -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(())
Expand Down Expand Up @@ -1940,15 +1904,15 @@ impl<'a, 'b> Parser<'a, 'b>
Ok(())
}

pub fn flags(&self) -> Iter<FlagBuilder> {
pub fn flags(&self) -> Iter<FlagBuilder<'a, 'b>> {
self.flags.iter()
}

pub fn opts(&self) -> Iter<OptBuilder> {
pub fn opts(&self) -> Iter<OptBuilder<'a, 'b>> {
self.opts.iter()
}

pub fn positionals(&self) -> vec_map::Values<PosBuilder> {
pub fn positionals(&self) -> vec_map::Values<PosBuilder<'a, 'b>> {
self.positionals.values()
}

Expand All @@ -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);
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/args/any_arg.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Std
use std::rc::Rc;
use std::fmt as std_fmt;

// Third Party
use vec_map::VecMap;
Expand All @@ -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<Vec<&'e str>>;
Expand Down
2 changes: 1 addition & 1 deletion src/completions/macros.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
4 changes: 2 additions & 2 deletions src/macros.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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); }
Expand All @@ -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); }
Expand Down

0 comments on commit 3d37001

Please sign in to comment.