Skip to content

Commit

Permalink
refactor(parser)!: Consolidate group/occurrence logic
Browse files Browse the repository at this point in the history 
We were independently starting occurrences and starting value groups.
Now we do them at the same time.

COMPATIBILITY: This changes us from counting occurrences per positional
when using `multiple_values` to one occurrence.  This is user visible
and tests were written against it but it goes against the documentation
and doesn't quite make sense.
  • Loading branch information
@epage
epage committed May 27, 2022
1 parent 28cb71c commit 9805fda
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 130 deletions.
38 changes: 6 additions & 32 deletions src/parser/arg_matcher.rs
View file
Expand Up @@ -11,9 +11,6 @@ use crate::parser::{ArgMatches, MatchedArg, SubCommand, ValueSource};
use crate::util::Id;
use crate::INTERNAL_ERROR_MSG;

// Third party
use indexmap::map::Entry;

#[derive(Debug, Default)]
pub(crate) struct ArgMatcher(ArgMatches);

Expand Down Expand Up @@ -141,6 +138,7 @@ impl ArgMatcher {
let ma = self.entry(id).or_insert(MatchedArg::new_arg(arg));
debug_assert_eq!(ma.type_id(), Some(arg.get_value_parser().type_id()));
ma.set_source(source);
ma.new_val_group();
}

pub(crate) fn start_custom_group(&mut self, id: &Id, source: ValueSource) {
Expand All @@ -151,6 +149,7 @@ impl ArgMatcher {
let ma = self.entry(id).or_insert(MatchedArg::new_group());
debug_assert_eq!(ma.type_id(), None);
ma.set_source(source);
ma.new_val_group();
}

pub(crate) fn start_occurrence_of_arg(&mut self, arg: &Arg) {
Expand All @@ -160,6 +159,7 @@ impl ArgMatcher {
debug_assert_eq!(ma.type_id(), Some(arg.get_value_parser().type_id()));
ma.set_source(ValueSource::CommandLine);
ma.inc_occurrences();
ma.new_val_group();
}

pub(crate) fn start_occurrence_of_group(&mut self, id: &Id) {
Expand All @@ -168,6 +168,7 @@ impl ArgMatcher {
debug_assert_eq!(ma.type_id(), None);
ma.set_source(ValueSource::CommandLine);
ma.inc_occurrences();
ma.new_val_group();
}

pub(crate) fn start_occurrence_of_external(&mut self, cmd: &crate::Command) {
Expand All @@ -184,46 +185,19 @@ impl ArgMatcher {
);
ma.set_source(ValueSource::CommandLine);
ma.inc_occurrences();
ma.new_val_group();
}

pub(crate) fn add_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString, append: bool) {
if append {
self.append_val_to(arg, val, raw_val);
} else {
self.push_val_to(arg, val, raw_val);
}
}

fn push_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString) {
// We will manually inc occurrences later(for flexibility under
// specific circumstances, like only add one occurrence for flag
// when we met: `--flag=one,two`).
let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG);
ma.push_val(val, raw_val);
}

fn append_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString) {
pub(crate) fn add_val_to(&mut self, arg: &Id, val: AnyValue, raw_val: OsString) {
let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG);
ma.append_val(val, raw_val);
}

pub(crate) fn new_val_group(&mut self, arg: &Id) {
let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG);
ma.new_val_group();
}

pub(crate) fn add_index_to(&mut self, arg: &Id, idx: usize) {
let ma = self.get_mut(arg).expect(INTERNAL_ERROR_MSG);
ma.push_index(idx);
}

pub(crate) fn has_val_groups(&mut self, arg: &Id) -> bool {
match self.entry(arg) {
Entry::Occupied(e) => e.get().has_val_groups(),
Entry::Vacant(_) => false,
}
}

pub(crate) fn needs_more_vals(&self, o: &Arg) -> bool {
debug!("ArgMatcher::needs_more_vals: o={}", o.name);
if let Some(ma) = self.get(&o.id) {
Expand Down
48 changes: 0 additions & 48 deletions src/parser/matches/matched_arg.rs
View file
Expand Up @@ -87,11 +87,6 @@ impl MatchedArg {
self.indices.push(index)
}

#[cfg(test)]
pub(crate) fn raw_vals(&self) -> Iter<Vec<OsString>> {
self.raw_vals.iter()
}

#[cfg(feature = "unstable-grouped")]
pub(crate) fn vals(&self) -> Iter<Vec<AnyValue>> {
self.vals.iter()
Expand All @@ -118,11 +113,6 @@ impl MatchedArg {
self.raw_vals_flatten().next()
}

pub(crate) fn push_val(&mut self, val: AnyValue, raw_val: OsString) {
self.vals.push(vec![val]);
self.raw_vals.push(vec![raw_val]);
}

pub(crate) fn new_val_group(&mut self) {
self.vals.push(vec![]);
self.raw_vals.push(vec![]);
Expand Down Expand Up @@ -151,10 +141,6 @@ impl MatchedArg {
self.vals.iter().flatten().count() == 0
}

pub(crate) fn has_val_groups(&self) -> bool {
!self.vals.is_empty()
}

pub(crate) fn check_explicit(&self, predicate: ArgPredicate) -> bool {
if self.source == Some(ValueSource::DefaultValue) {
return false;
Expand Down Expand Up @@ -244,38 +230,4 @@ mod tests {
m.append_val(AnyValue::new(String::from("ccc")), "ccc".into());
assert_eq!(m.first_raw(), Some(&OsString::from("bbb")));
}

#[test]
fn test_grouped_vals_push_and_append() {
let mut m = MatchedArg::new_group();
m.push_val(AnyValue::new(String::from("aaa")), "aaa".into());
m.new_val_group();
m.append_val(AnyValue::new(String::from("bbb")), "bbb".into());
m.append_val(AnyValue::new(String::from("ccc")), "ccc".into());
m.new_val_group();
m.append_val(AnyValue::new(String::from("ddd")), "ddd".into());
m.push_val(AnyValue::new(String::from("eee")), "eee".into());
m.new_val_group();
m.append_val(AnyValue::new(String::from("fff")), "fff".into());
m.append_val(AnyValue::new(String::from("ggg")), "ggg".into());
m.append_val(AnyValue::new(String::from("hhh")), "hhh".into());
m.append_val(AnyValue::new(String::from("iii")), "iii".into());

let vals: Vec<&Vec<OsString>> = m.raw_vals().collect();
assert_eq!(
vals,
vec![
&vec![OsString::from("aaa")],
&vec![OsString::from("bbb"), OsString::from("ccc"),],
&vec![OsString::from("ddd")],
&vec![OsString::from("eee")],
&vec![
OsString::from("fff"),
OsString::from("ggg"),
OsString::from("hhh"),
OsString::from("iii"),
]
]
)
}
}
52 changes: 12 additions & 40 deletions src/parser/parser.rs
View file
Expand Up @@ -347,7 +347,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
&self.cmd[id],
arg_os.to_value_os(),
matcher,
true,
trailing_values,
)?;
parse_state = match parse_result {
Expand All @@ -374,15 +373,10 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
trailing_values = true;
}

// Increase occurrence no matter if we are appending, occurrences
// of positional argument equals to number of values rather than
// the number of value groups.
self.start_occurrence_of_arg(matcher, p);
// Creating new value group rather than appending when the arg
// doesn't have any value. This behaviour is right because
// positional arguments are always present continuously.
let append = self.has_val_groups(matcher, p);
self.add_val_to_arg(p, arg_os.to_value_os(), matcher, append, trailing_values)?;
if !p.is_multiple_values_set() || !matcher.contains(&p.id) {
self.start_occurrence_of_arg(matcher, p);
}
self.add_val_to_arg(p, arg_os.to_value_os(), matcher, trailing_values)?;

// Only increment the positional counter if it doesn't allow multiples
if !p.is_multiple() {
Expand Down Expand Up @@ -415,7 +409,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
for raw_val in raw_args.remaining(&mut args_cursor) {
let val = external_parser.parse_ref(self.cmd, None, raw_val)?;
let external_id = &Id::empty_hash();
sc_m.add_val_to(external_id, val, raw_val.to_os_string(), false);
sc_m.add_val_to(external_id, val, raw_val.to_os_string());
}

matcher.subcommand(SubCommand {
Expand Down Expand Up @@ -951,7 +945,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
opt,
opt.default_missing_vals.iter().map(OsString::from),
matcher,
false,
)?;
};
if attached_value.is_some() {
Expand All @@ -967,15 +960,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
}
} else if let Some(v) = attached_value {
self.start_occurrence_of_arg(matcher, opt);
self.add_val_to_arg(opt, v, matcher, false, trailing_values)?;
self.add_val_to_arg(opt, v, matcher, trailing_values)?;
Ok(ParseResult::ValuesDone)
} else {
debug!("Parser::parse_opt: More arg vals required...");
self.start_occurrence_of_arg(matcher, opt);
matcher.new_val_group(&opt.id);
for group in self.cmd.groups_for_arg(&opt.id) {
matcher.new_val_group(&group);
}
Ok(ParseResult::Opt(opt.id.clone()))
}
}
Expand All @@ -985,7 +974,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
arg: &Arg<'help>,
val: &RawOsStr,
matcher: &mut ArgMatcher,
append: bool,
trailing_values: bool,
) -> ClapResult<ParseResult> {
debug!("Parser::add_val_to_arg; arg={}, val={:?}", arg.name, val);
Expand All @@ -1001,7 +989,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
.split(delim)
.map(|x| x.to_os_str().into_owned())
.take_while(|val| Some(val.as_os_str()) != terminator);
self.add_multiple_vals_to_arg(arg, vals, matcher, append)?;
self.add_multiple_vals_to_arg(arg, vals, matcher)?;
// If there was a delimiter used or we must use the delimiter to
// separate the values or no more vals is needed, we're not
// looking for more values.
Expand All @@ -1020,7 +1008,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
return Ok(ParseResult::ValuesDone);
}
}
self.add_single_val_to_arg(arg, val.to_os_str().into_owned(), matcher, append)?;
self.add_single_val_to_arg(arg, val.to_os_str().into_owned(), matcher)?;
if matcher.needs_more_vals(arg) {
Ok(ParseResult::Opt(arg.id.clone()))
} else {
Expand All @@ -1033,17 +1021,9 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
arg: &Arg<'help>,
raw_vals: impl Iterator<Item = OsString>,
matcher: &mut ArgMatcher,
append: bool,
) -> ClapResult<()> {
// If not appending, create a new val group and then append vals in.
if !append {
matcher.new_val_group(&arg.id);
for group in self.cmd.groups_for_arg(&arg.id) {
matcher.new_val_group(&group);
}
}
for raw_val in raw_vals {
self.add_single_val_to_arg(arg, raw_val, matcher, true)?;
self.add_single_val_to_arg(arg, raw_val, matcher)?;
}

Ok(())
Expand All @@ -1054,7 +1034,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
arg: &Arg<'help>,
raw_val: OsString,
matcher: &mut ArgMatcher,
append: bool,
) -> ClapResult<()> {
debug!("Parser::add_single_val_to_arg: adding val...{:?}", raw_val);

Expand All @@ -1069,19 +1048,15 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {

// Increment or create the group "args"
for group in self.cmd.groups_for_arg(&arg.id) {
matcher.add_val_to(&group, val.clone(), raw_val.clone(), append);
matcher.add_val_to(&group, val.clone(), raw_val.clone());
}

matcher.add_val_to(&arg.id, val, raw_val, append);
matcher.add_val_to(&arg.id, val, raw_val);
matcher.add_index_to(&arg.id, self.cur_idx.get());

Ok(())
}

fn has_val_groups(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) -> bool {
matcher.has_val_groups(&arg.id)
}

fn parse_flag(
&self,
ident: Identifier,
Expand Down Expand Up @@ -1164,7 +1139,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
val, trailing_values
);
self.start_custom_arg(matcher, arg, ValueSource::EnvVariable);
self.add_val_to_arg(arg, &val, matcher, false, trailing_values)?;
self.add_val_to_arg(arg, &val, matcher, trailing_values)?;
} else {
match arg.get_action() {
Action::StoreValue => unreachable!("{:?} is not a flag", arg.get_id()),
Expand Down Expand Up @@ -1236,7 +1211,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
arg,
&RawOsStr::new(default),
matcher,
false,
trailing_values,
)?;
}
Expand Down Expand Up @@ -1279,7 +1253,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
arg,
process_default_vals(arg, &arg.default_vals).into_iter(),
matcher,
false,
)?;
}
} else {
Expand Down Expand Up @@ -1307,7 +1280,6 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
arg,
process_default_vals(arg, &arg.default_missing_vals).into_iter(),
matcher,
false,
)?;
}
None => {
Expand Down

0 comments on commit 9805fda

Please sign in to comment.