refactor(validator): cleanup code (#2512)

* refactor(validator): use filtermap style for `gather_conflicts` and `validate_exclusive`

* refactor(validator): remove nested `filter`s in `build_conflict_err_usage`

* refactor(validator): add more code beautifying

* refactor(validator): revert some unnecessary changes

* refactor(validator): revert some unnecessary changes, take 2
This commit is contained in:
rami3l 2021-06-07 01:45:45 +02:00 committed by GitHub
parent dc8d0f1b08
commit 6e158d9f8d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23

View file

@ -180,17 +180,18 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
let retained_blacklist = &retained_arg.blacklist;
let used_filtered: Vec<Id> = matcher
.arg_names()
.filter(|key| *key != conflicting_key)
.filter(|key| !retained_blacklist.contains(key))
.filter(|key| *key != conflicting_key && !retained_blacklist.contains(key))
.cloned()
.collect();
let required: Vec<Id> = used_filtered
.iter()
.filter_map(|key| self.p.app.find(key))
.flat_map(|key_arg| key_arg.requires.iter().map(|item| &item.1))
.filter(|item| !used_filtered.contains(item))
.filter(|key| *key != conflicting_key)
.filter(|key| !retained_blacklist.contains(key))
.flat_map(|arg| arg.requires.iter().map(|item| &item.1))
.filter(|key| {
!used_filtered.contains(key)
&& *key != conflicting_key
&& !retained_blacklist.contains(key)
})
.chain(used_filtered.iter())
.cloned()
.collect();
@ -200,28 +201,32 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
fn build_conflict_err(&self, name: &Id, matcher: &ArgMatcher) -> ClapResult<()> {
debug!("Validator::build_conflict_err: name={:?}", name);
if let Some(checked_arg) = self.p.app.find(name) {
for k in matcher.arg_names() {
if let Some(a) = self.p.app.find(k) {
if a.blacklist.contains(&name) {
let (_former, former_arg, latter, latter_arg) = {
let name_pos = matcher.arg_names().position(|key| key == name);
let k_pos = matcher.arg_names().position(|key| key == k);
if name_pos < k_pos {
(name, checked_arg, k, a)
} else {
(k, a, name, checked_arg)
}
};
let usg = self.build_conflict_err_usage(matcher, former_arg, latter);
return Err(Error::argument_conflict(
latter_arg,
Some(former_arg.to_string()),
usg,
self.p.app.color(),
));
}
}
}
matcher
.arg_names()
.filter_map(|k| {
let arg = self.p.app.find(k);
// For an arg that blacklists `name`, this will return `Some((k, a))` to indicate a conflict.
arg.filter(|a| a.blacklist.contains(&name)).map(|a| (k, a))
})
.try_for_each(|(k, a)| {
// The error will be then constructed according to the first conflict.
let (_former, former_arg, latter, latter_arg) = {
let name_pos = matcher.arg_names().position(|key| key == name);
let k_pos = matcher.arg_names().position(|key| key == k);
if name_pos < k_pos {
(name, checked_arg, k, a)
} else {
(k, a, name, checked_arg)
}
};
let usg = self.build_conflict_err_usage(matcher, former_arg, latter);
Err(Error::argument_conflict(
latter_arg,
Some(former_arg.to_string()),
usg,
self.p.app.color(),
))
})
} else if let Some(g) = self.p.app.groups.iter().find(|x| x.id == *name) {
let usg = Usage::new(self.p).create_usage_with_title(&[]);
let args_in_group = self.p.app.unroll_args_in_group(&g.id);
@ -234,13 +239,15 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
.find(|x| x != &first && args_in_group.contains(x))
.map(|x| self.p.app[x].to_string());
debug!("Validator::build_conflict_err: c_with={:?}:group", c_with);
return Err(Error::argument_conflict(
Err(Error::argument_conflict(
&self.p.app[first],
c_with,
usg,
self.p.app.color(),
));
}
))
} else {
Ok(())
}?;
panic!("{}", INTERNAL_ERROR_MSG);
}
@ -250,131 +257,136 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
self.validate_exclusive(matcher)?;
self.gather_conflicts(matcher);
for name in self.c.iter() {
debug!("Validator::validate_conflicts:iter:{:?}", name);
let mut should_err = false;
if let Some(g) = self
.p
.app
.groups
.iter()
.find(|g| !g.multiple && &g.id == name)
{
let conf_with_self = self
self.c
.iter()
.filter(|&name| {
debug!("Validator::validate_conflicts:iter:{:?}", name);
// Filter out the conflicting cases.
if let Some(g) = self
.p
.app
.unroll_args_in_group(&g.id)
.groups
.iter()
.filter(|&a| matcher.contains(a))
.count()
> 1;
let conf_with_arg = g.conflicts.iter().any(|x| matcher.contains(x));
let arg_conf_with_gr = matcher
.arg_names()
.filter_map(|x| self.p.app.find(x))
.any(|x| x.blacklist.iter().any(|c| *c == g.id));
should_err = conf_with_self || conf_with_arg || arg_conf_with_gr;
} else if let Some(ma) = matcher.get(name) {
debug!(
"Validator::validate_conflicts:iter:{:?}: matcher contains it...",
name
);
should_err = ma.occurs > 0;
}
if should_err {
return self.build_conflict_err(name, matcher);
}
}
Ok(())
.find(|g| !g.multiple && g.id == *name)
{
let conf_with_self = || {
self.p
.app
.unroll_args_in_group(&g.id)
.iter()
.filter(|&a| matcher.contains(a))
.count()
> 1
};
let conf_with_arg = || g.conflicts.iter().any(|x| matcher.contains(x));
let arg_conf_with_gr = || {
matcher
.arg_names()
.filter_map(|x| self.p.app.find(x))
.any(|x| x.blacklist.iter().any(|c| *c == g.id))
};
conf_with_self() || conf_with_arg() || arg_conf_with_gr()
} else if let Some(ma) = matcher.get(name) {
debug!(
"Validator::validate_conflicts:iter:{:?}: matcher contains it...",
name
);
ma.occurs > 0
} else {
false
}
})
// Throw an error for the first conflict found.
.try_for_each(|odd| self.build_conflict_err(odd, matcher))
}
fn validate_exclusive(&mut self, matcher: &mut ArgMatcher) -> ClapResult<()> {
debug!("Validator::validate_exclusive");
let args_count = matcher.arg_names().count();
for name in matcher.arg_names() {
debug!("Validator::validate_exclusive:iter:{:?}", name);
if let Some(arg) = self.p.app.find(name) {
if arg.exclusive && args_count > 1 {
let c_with: Option<String> = None;
return Err(Error::argument_conflict(
arg,
c_with,
Usage::new(self.p).create_usage_with_title(&[]),
self.p.app.color(),
));
}
}
}
Ok(())
matcher
.arg_names()
.filter_map(|name| {
debug!("Validator::validate_exclusive:iter:{:?}", name);
self.p
.app
.find(name)
// Find `arg`s which are exclusive but also appear with other args.
.filter(|&arg| arg.exclusive && args_count > 1)
})
// Throw an error for the first conflict found.
.try_for_each(|arg| {
Err(Error::argument_conflict(
arg,
None,
Usage::new(self.p).create_usage_with_title(&[]),
self.p.app.color(),
))
})
}
// Gathers potential conflicts based on used argument, but without considering requirements
// and such
fn gather_conflicts(&mut self, matcher: &mut ArgMatcher) {
debug!("Validator::gather_conflicts");
for name in matcher.arg_names() {
debug!("Validator::gather_conflicts:iter: id={:?}", name);
// if arg is "present" only because it got default value
// it doesn't conflict with anything
//
// TODO: @refactor Do it in a more elegant way
if matcher
.get(name)
.map_or(false, |a| a.ty == ValueType::DefaultValue)
{
debug!("Validator::gather_conflicts:iter: This is default value, skipping.",);
continue;
}
if let Some(arg) = self.p.app.find(name) {
// Since an arg was used, every arg it conflicts with is added to the conflicts
for conf in &arg.blacklist {
if self.p.app.find(conf).is_some() {
if conf != name {
self.c.insert(conf.clone());
matcher
.arg_names()
.filter(|name| {
debug!("Validator::gather_conflicts:iter: id={:?}", name);
// if arg is "present" only because it got default value
// it doesn't conflict with anything and should be skipped
let skip = matcher
.get(name)
.map_or(false, |a| a.ty == ValueType::DefaultValue);
if skip {
debug!("Validator::gather_conflicts:iter: This is default value, skipping.",);
}
!skip
})
.for_each(|name| {
if let Some(arg) = self.p.app.find(name) {
// Since an arg was used, every arg it conflicts with is added to the conflicts
for conf in &arg.blacklist {
/*
for g_arg in self.p.app.unroll_args_in_group(&g.name) {
if &g_arg != name {
self.c.insert(g.id.clone()); // TODO ERROR is here - groups allow one arg but this line disallows all group args
}
}
} else {
// for g_arg in self.p.app.unroll_args_in_group(conf) {
// if &g_arg != name {
self.c.insert(conf.clone()); // TODO ERROR is here - groups allow one arg but this line disallows all group args
// }
// }
*/
self.c.insert(conf.clone());
}
}
// Now we need to know which groups this arg was a member of, to add all other
// args in that group to the conflicts, as well as any args those args conflict
// with
for grp in self.p.app.groups_for_arg(&name) {
if let Some(g) = self
.p
.app
.groups
.iter()
.find(|g| !g.multiple && g.id == grp)
{
// for g_arg in self.p.app.unroll_args_in_group(&g.name) {
// if &g_arg != name {
self.c.insert(g.id.clone());
// }
// }
// Now we need to know which groups this arg was a member of, to add all other
// args in that group to the conflicts, as well as any args those args conflict
// with
for grp in self.p.app.groups_for_arg(&name) {
if let Some(g) = self
.p
.app
.groups
.iter()
.find(|g| !g.multiple && g.id == grp)
{
/*
for g_arg in self.p.app.unroll_args_in_group(&g.name) {
if &g_arg != name {
self.c.insert(g.id.clone());
}
}
*/
self.c.insert(g.id.clone());
}
}
} else if let Some(g) = self
.p
.app
.groups
.iter()
.find(|g| !g.multiple && g.id == *name)
{
debug!("Validator::gather_conflicts:iter:{:?}:group", name);
self.c.insert(g.id.clone());
}
} else if let Some(g) = self
.p
.app
.groups
.iter()
.find(|g| !g.multiple && g.id == *name)
{
debug!("Validator::gather_conflicts:iter:{:?}:group", name);
self.c.insert(g.id.clone());
}
}
});
}
fn gather_requirements(&mut self, matcher: &ArgMatcher) {
@ -396,7 +408,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
fn validate_matched_args(&self, matcher: &mut ArgMatcher) -> ClapResult<()> {
debug!("Validator::validate_matched_args");
for (name, ma) in matcher.iter() {
matcher.iter().try_for_each(|(name, ma)| {
debug!(
"Validator::validate_matched_args:iter:{:?}: vals={:#?}",
name,
@ -419,8 +431,8 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
return self.missing_required_error(matcher, vec![name.clone()]);
}
}
}
Ok(())
Ok(())
})
}
fn validate_arg_num_occurs(&self, a: &Arg, ma: &MatchedArg) -> ClapResult<()> {
@ -428,7 +440,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
"Validator::validate_arg_num_occurs: {:?}={}",
a.name, ma.occurs
);
// Occurence of positional argument equals to number of values rather
// Occurrence of positional argument equals to number of values rather
// than number of grouped values.
if ma.occurs > 1 && !a.is_set(ArgSettings::MultipleOccurrences) && !a.is_positional() {
// Not the first time, and we don't allow multiples
@ -569,19 +581,10 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
}
}
let mut match_all = true;
for (other, val) in &a.r_ifs_all {
if let Some(ma) = matcher.get(other) {
if !ma.contains_val(val) {
match_all = false;
break;
}
} else {
match_all = false;
break;
}
}
let match_all = a
.r_ifs_all
.iter()
.all(|(other, val)| matcher.get(other).map_or(false, |ma| ma.contains_val(val)));
if match_all && !a.r_ifs_all.is_empty() && !matcher.contains(&a.id) {
return self.missing_required_error(matcher, vec![a.id.clone()]);
}
@ -615,9 +618,11 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
.app
.args
.args()
.filter(|a| !a.r_unless.is_empty())
.filter(|a| !matcher.contains(&a.id))
.filter(|a| self.fails_arg_required_unless(a, matcher))
.filter(|&a| {
!a.r_unless.is_empty()
&& !matcher.contains(&a.id)
&& self.fails_arg_required_unless(a, matcher)
})
.map(|a| a.id.clone())
.collect();
if failed_args.is_empty() {
@ -669,7 +674,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> {
Err(Error::missing_required_argument(
req_args,
usg.create_usage_with_title(&*used),
usg.create_usage_with_title(&used),
self.p.app.color(),
))
}