Finish arg grouping logic

src/parse/arg_matcher.rs

@@ -8,6 +8,9 @@ use crate::{
     util::Id,
 };
 
+// Third party
+use indexmap::map::Entry;
+
 #[derive(Debug)]
 pub(crate) struct ArgMatcher(pub(crate) ArgMatches);
 
@@ -121,7 +124,15 @@ impl ArgMatcher {
         ma.occurs += 1;
     }
 
-    pub(crate) fn add_val_to(&mut self, arg: &Id, val: OsString, ty: ValueType) {
+    pub(crate) fn add_val_to(&mut self, arg: &Id, val: OsString, ty: ValueType, append: bool) {
+        if append {
+            self.append_val_to(arg, val, ty);
+        } else {
+            self.push_val_to(arg, val, ty);
+        }
+    }
+
+    pub(crate) fn push_val_to(&mut self, arg: &Id, val: OsString, ty: ValueType) {
         // We will manually inc occurrences later(for flexibility under
         // specific circumstances, like only add one occurrence for flag
         // when we met: `--flag=one,two`).
@@ -130,15 +141,8 @@ impl ArgMatcher {
         ma.push_val(val);
     }
 
-    pub(crate) fn add_vals_to(&mut self, arg: &Id, vals: Vec<OsString>, ty: ValueType) {
+    pub(crate) fn new_val_group(&mut self, arg: &Id) {
         let ma = self.entry(arg).or_default();
-        ma.set_ty(ty);
-        ma.push_vals(vals);
-    }
-
-    pub(crate) fn add_val_group_to(&mut self, arg: &Id, ty: ValueType) {
-        let ma = self.entry(arg).or_default();
-        ma.set_ty(ty);
         ma.new_val_group();
     }
 
@@ -154,19 +158,24 @@ impl ArgMatcher {
         ma.push_index(idx);
     }
 
+    pub(crate) fn arg_have_val(&mut self, arg: &Id) -> bool {
+        matches!(self.entry(arg), Entry::Occupied(_))
+    }
+
     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) {
+            let current_num = ma.num_vals_current_group() as u64;
             if let Some(num) = o.num_vals {
                 debug!("ArgMatcher::needs_more_vals: num_vals...{}", num);
                 return if o.is_set(ArgSettings::MultipleValues) {
-                    ((ma.num_vals() as u64) % num) != 0
+                    (current_num % num) != 0
                 } else {
-                    num != (ma.num_vals() as u64)
+                    num != current_num
                 };
             } else if let Some(num) = o.max_vals {
                 debug!("ArgMatcher::needs_more_vals: max_vals...{}", num);
-                return (ma.num_vals() as u64) < num;
+                return current_num < num;
             } else if o.min_vals.is_some() {
                 debug!("ArgMatcher::needs_more_vals: min_vals...true");
                 return true;

src/parse/matches/matched_arg.rs

@@ -77,20 +77,37 @@ impl MatchedArg {
         self.vals.last_mut().expect(INTERNAL_ERROR_MSG).push(val)
     }
 
-    pub(crate) fn push_vals(&mut self, vals: Vec<OsString>) {
-        self.vals.push(vals)
-    }
-
     pub(crate) fn num_vals(&self) -> usize {
         self.vals.iter().flatten().count()
     }
 
+    pub(crate) fn num_vals_current_group(&self) -> usize {
+        self.vals.last().map(|x| x.len()).unwrap_or(0)
+    }
+
     pub(crate) fn no_val(&self) -> bool {
         self.vals.iter().flatten().count() == 0
     }
 
     pub(crate) fn remove_vals(&mut self, len: usize) {
-        self.vals.drain(0..len);
+        let mut want_remove = len;
+        let mut remove_group = None;
+        let mut remove_val = None;
+        for (i, g) in self.vals().enumerate() {
+            if g.len() <= want_remove {
+                want_remove -= g.len();
+                remove_group = Some(i);
+            } else {
+                remove_val = Some(want_remove);
+                break;
+            }
+        }
+        if let Some(remove_group) = remove_group {
+            self.vals.drain(0..=remove_group);
+        }
+        if let Some(remove_val) = remove_val {
+            self.vals[0].drain(0..remove_val);
+        }
     }
 
     pub(crate) fn contains_val(&self, val: &str) -> bool {
@@ -102,3 +119,191 @@ impl MatchedArg {
         self.ty = ty;
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn test_grouped_vals_push_and_append() {
+        let mut m = MatchedArg::new();
+        m.push_val("aaa".into());
+        m.new_val_group();
+        m.append_val("bbb".into());
+        m.append_val("ccc".into());
+        m.new_val_group();
+        m.append_val("ddd".into());
+        m.push_val("eee".into());
+        m.new_val_group();
+        m.append_val("fff".into());
+        m.append_val("ggg".into());
+        m.append_val("hhh".into());
+        m.append_val("iii".into());
+
+        let vals: Vec<&Vec<OsString>> = m.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"),
+                ]
+            ]
+        )
+    }
+
+    #[test]
+    fn test_grouped_vals_removal() {
+        let m = {
+            let mut m = MatchedArg::new();
+            m.push_val("aaa".into());
+            m.new_val_group();
+            m.append_val("bbb".into());
+            m.append_val("ccc".into());
+            m.new_val_group();
+            m.append_val("ddd".into());
+            m.push_val("eee".into());
+            m.new_val_group();
+            m.append_val("fff".into());
+            m.append_val("ggg".into());
+            m.append_val("hhh".into());
+            m.append_val("iii".into());
+            m
+        };
+        {
+            let mut m = m.clone();
+            m.remove_vals(0);
+            let vals1 = m.vals().collect::<Vec<_>>();
+            assert_eq!(
+                vals1,
+                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"),
+                    ]
+                ]
+            );
+        }
+
+        {
+            let mut m = m.clone();
+            m.remove_vals(1);
+            let vals0 = m.vals().collect::<Vec<_>>();
+            assert_eq!(
+                vals0,
+                vec![
+                    &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"),
+                    ]
+                ]
+            );
+        }
+
+        {
+            let mut m = m.clone();
+            m.remove_vals(2);
+            let vals1 = m.vals().collect::<Vec<_>>();
+            assert_eq!(
+                vals1,
+                vec![
+                    &vec![OsString::from("ccc"),],
+                    &vec![OsString::from("ddd")],
+                    &vec![OsString::from("eee")],
+                    &vec![
+                        OsString::from("fff"),
+                        OsString::from("ggg"),
+                        OsString::from("hhh"),
+                        OsString::from("iii"),
+                    ]
+                ]
+            );
+        }
+
+        {
+            let mut m = m.clone();
+            m.remove_vals(3);
+            let vals1 = m.vals().collect::<Vec<_>>();
+            assert_eq!(
+                vals1,
+                vec![
+                    &vec![OsString::from("ddd")],
+                    &vec![OsString::from("eee")],
+                    &vec![
+                        OsString::from("fff"),
+                        OsString::from("ggg"),
+                        OsString::from("hhh"),
+                        OsString::from("iii"),
+                    ]
+                ]
+            );
+        }
+
+        {
+            let mut m = m.clone();
+            m.remove_vals(4);
+            let vals1 = m.vals().collect::<Vec<_>>();
+            assert_eq!(
+                vals1,
+                vec![
+                    &vec![OsString::from("eee")],
+                    &vec![
+                        OsString::from("fff"),
+                        OsString::from("ggg"),
+                        OsString::from("hhh"),
+                        OsString::from("iii"),
+                    ]
+                ]
+            );
+        }
+
+        {
+            let mut m = m.clone();
+            m.remove_vals(5);
+            let vals1 = m.vals().collect::<Vec<_>>();
+            assert_eq!(
+                vals1,
+                vec![&vec![
+                    OsString::from("fff"),
+                    OsString::from("ggg"),
+                    OsString::from("hhh"),
+                    OsString::from("iii"),
+                ]]
+            );
+        }
+
+        {
+            let mut m = m.clone();
+            m.remove_vals(7);
+            let vals1 = m.vals().collect::<Vec<_>>();
+            assert_eq!(
+                vals1,
+                vec![&vec![OsString::from("hhh"), OsString::from("iii"),]]
+            );
+        }
+
+        {
+            let mut m = m.clone();
+            m.remove_vals(9);
+            let vals1: Vec<&Vec<OsString>> = m.vals().collect();
+            assert!(vals1.is_empty());
+        }
+    }
+}

src/parse/parser.rs

@@ -435,6 +435,7 @@ impl<'help, 'app> Parser<'help, 'app> {
                         &arg_os,
                         matcher,
                         ValueType::CommandLine,
+                        true,
                     );
                     // get the next value from the iterator
                     continue;
@@ -521,7 +522,8 @@ impl<'help, 'app> Parser<'help, 'app> {
                 }
 
                 self.seen.push(p.id.clone());
-                self.add_val_to_arg(p, &arg_os, matcher, ValueType::CommandLine);
+                let append = self.arg_have_val(matcher, p);
+                self.add_val_to_arg(p, &arg_os, matcher, ValueType::CommandLine, append);
 
                 matcher.inc_occurrence_of(&p.id);
                 for grp in self.app.groups_for_arg(&p.id) {
@@ -558,7 +560,12 @@ impl<'help, 'app> Parser<'help, 'app> {
                             self.app.color(),
                         ));
                     }
-                    sc_m.add_val_to(&Id::empty_hash(), v.to_os_string(), ValueType::CommandLine);
+                    sc_m.add_val_to(
+                        &Id::empty_hash(),
+                        v.to_os_string(),
+                        ValueType::CommandLine,
+                        false,
+                    );
                 }
 
                 matcher.subcommand(SubCommand {
@@ -1167,7 +1174,7 @@ impl<'help, 'app> Parser<'help, 'app> {
                 fv,
                 fv.starts_with("=")
             );
-            self.add_val_to_arg(opt, &v, matcher, ValueType::CommandLine);
+            self.add_val_to_arg(opt, &v, matcher, ValueType::CommandLine, false);
         } else if require_equals && !empty_vals && !min_vals_zero {
             debug!("None, but requires equals...Error");
             return Err(ClapError::empty_value(
@@ -1179,17 +1186,12 @@ impl<'help, 'app> Parser<'help, 'app> {
             debug!("None and requires equals, but min_vals == 0");
             if !opt.default_missing_vals.is_empty() {
                 debug!("Parser::parse_opt: has default_missing_vals");
-                let default_missing_vals = opt
+                let vals = opt
                     .default_missing_vals
                     .iter()
-                    .map(|x| ArgStr::new(x))
+                    .map(OsString::from)
                     .collect();
-                self.add_multiple_vals_to_arg(
-                    opt,
-                    default_missing_vals,
-                    matcher,
-                    ValueType::CommandLine,
-                );
+                self.add_multiple_vals_to_arg(opt, vals, matcher, ValueType::CommandLine, false);
             };
         } else {
             debug!("None");
@@ -1211,6 +1213,10 @@ impl<'help, 'app> Parser<'help, 'app> {
             || (multiple && !needs_delimiter) && !has_eq && matcher.needs_more_vals(opt)
         {
             debug!("Parser::parse_opt: More arg vals required...");
+            matcher.new_val_group(&opt.id);
+            for group in self.app.groups_for_arg(&opt.id) {
+                matcher.new_val_group(&group);
+            }
             Ok(ParseResult::Opt(opt.id.clone()))
         } else {
             debug!("Parser::parse_opt: More arg vals not required...");
@@ -1224,6 +1230,7 @@ impl<'help, 'app> Parser<'help, 'app> {
         val: &ArgStr,
         matcher: &mut ArgMatcher,
         ty: ValueType,
+        append: bool,
     ) -> ParseResult {
         debug!("Parser::add_val_to_arg; arg={}, val={:?}", arg.name, val);
         debug!(
@@ -1246,16 +1253,16 @@ impl<'help, 'app> Parser<'help, 'app> {
                 } else {
                     arg_split.into_iter().collect()
                 };
-                let mut parse_result = if vals.is_empty() {
-                    ParseResult::ValuesDone
-                } else {
-                    self.add_multiple_vals_to_arg(arg, vals, matcher, ty)
-                };
+                let vals = vals.into_iter().map(|x| x.to_os_string()).collect();
+                self.add_multiple_vals_to_arg(arg, vals, matcher, ty, append);
                 // If there was a delimiter used or we must use the delimiter to
                 // separate the values, we're not looking for more values.
-                if val.contains_char(delim) || arg.is_set(ArgSettings::RequireDelimiter) {
-                    parse_result = ParseResult::ValuesDone;
-                }
+                let parse_result =
+                    if val.contains_char(delim) || arg.is_set(ArgSettings::RequireDelimiter) {
+                        ParseResult::ValuesDone
+                    } else {
+                        self.need_more_vals(matcher, arg)
+                    };
                 return parse_result;
             }
         }
@@ -1264,59 +1271,50 @@ impl<'help, 'app> Parser<'help, 'app> {
                 return ParseResult::ValuesDone;
             }
         }
-        self.add_single_val_to_arg(arg, val, matcher, ty)
+        self.add_single_val_to_arg(arg, val.to_os_string(), matcher, ty, append);
+        self.need_more_vals(matcher, arg)
     }
 
     fn add_multiple_vals_to_arg(
         &self,
         arg: &Arg<'help>,
-        vals: Vec<ArgStr>,
+        vals: Vec<OsString>,
         matcher: &mut ArgMatcher,
         ty: ValueType,
-    ) -> ParseResult {
-        debug!("Parser::add_multiple_val_to_arg: adding vals...{:?}", vals);
-
-        let num_vals = vals.len();
-        let begin_index = self.cur_idx.get() + 1;
-        self.cur_idx.set(self.cur_idx.get() + num_vals);
-        for i in begin_index..begin_index + num_vals {
-            matcher.add_index_to(&arg.id, i, ty);
+        append: bool,
+    ) {
+        // If not appending, create a new val group and then append vals in.
+        if !append {
+            matcher.new_val_group(&arg.id);
         }
-        let vals: Vec<_> = vals.into_iter().map(|x| x.to_os_string()).collect();
-        // Increment or create the group "args"
-        for val in vals.iter() {
-            for group in self.app.groups_for_arg(&arg.id) {
-                matcher.add_val_to(&group, val.clone(), ty);
-            }
-        }
-        matcher.add_vals_to(&arg.id, vals, ty);
-        if matcher.needs_more_vals(arg) {
-            ParseResult::Opt(arg.id.clone())
-        } else {
-            ParseResult::ValuesDone
+        for val in vals {
+            self.add_single_val_to_arg(arg, val.clone(), matcher, ty, true);
         }
     }
 
     fn add_single_val_to_arg(
         &self,
         arg: &Arg<'help>,
-        val: &ArgStr,
+        val: OsString,
         matcher: &mut ArgMatcher,
         ty: ValueType,
-    ) -> ParseResult {
+        append: bool,
+    ) {
         debug!("Parser::add_single_val_to_arg: adding val...{:?}", val);
 
         // update the current index because each value is a distinct index to clap
         self.cur_idx.set(self.cur_idx.get() + 1);
 
-        matcher.add_val_to(&arg.id, val.to_os_string(), ty);
-        matcher.add_index_to(&arg.id, self.cur_idx.get(), ty);
-
         // Increment or create the group "args"
         for group in self.app.groups_for_arg(&arg.id) {
-            matcher.add_val_to(&group, val.to_os_string(), ty);
+            matcher.add_val_to(&group, val.clone(), ty, append);
         }
 
+        matcher.add_val_to(&arg.id, val, ty, append);
+        matcher.add_index_to(&arg.id, self.cur_idx.get(), ty);
+    }
+
+    fn need_more_vals(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) -> ParseResult {
         if matcher.needs_more_vals(arg) {
             ParseResult::Opt(arg.id.clone())
         } else {
@@ -1324,6 +1322,10 @@ impl<'help, 'app> Parser<'help, 'app> {
         }
     }
 
+    fn arg_have_val(&self, matcher: &mut ArgMatcher, arg: &Arg<'help>) -> bool {
+        matcher.arg_have_val(&arg.id)
+    }
+
     fn parse_flag(&self, flag: &Arg<'help>, matcher: &mut ArgMatcher) -> ParseResult {
         debug!("Parser::parse_flag");
 
@@ -1437,7 +1439,7 @@ impl<'help, 'app> Parser<'help, 'app> {
                     };
 
                     if add {
-                        self.add_val_to_arg(arg, &ArgStr::new(default), matcher, ty);
+                        self.add_val_to_arg(arg, &ArgStr::new(default), matcher, ty, false);
                         return;
                     }
                 }
@@ -1454,9 +1456,8 @@ impl<'help, 'app> Parser<'help, 'app> {
             } else {
                 debug!("Parser::add_value:iter:{}: wasn't used", arg.name);
 
-                for val in &arg.default_vals {
-                    self.add_val_to_arg(arg, &ArgStr::new(val), matcher, ty);
-                }
+                let vals = arg.default_vals.iter().map(OsString::from).collect();
+                self.add_multiple_vals_to_arg(arg, vals, matcher, ty, false);
             }
         } else {
             debug!(
@@ -1478,9 +1479,12 @@ impl<'help, 'app> Parser<'help, 'app> {
                         "Parser::add_value:iter:{}: has no user defined vals",
                         arg.name
                     );
-                    for val in &arg.default_missing_vals {
-                        self.add_val_to_arg(arg, &ArgStr::new(val), matcher, ty);
-                    }
+                    let vals = arg
+                        .default_missing_vals
+                        .iter()
+                        .map(OsString::from)
+                        .collect();
+                    self.add_multiple_vals_to_arg(arg, vals, matcher, ty, false);
                 }
                 None => {
                     debug!("Parser::add_value:iter:{}: wasn't used", arg.name);
@@ -1508,7 +1512,7 @@ impl<'help, 'app> Parser<'help, 'app> {
                 if let Some((_, Some(ref val))) = a.env {
                     let val = &ArgStr::new(val);
                     if a.is_set(ArgSettings::TakesValue) {
-                        self.add_val_to_arg(a, val, matcher, ValueType::EnvVariable);
+                        self.add_val_to_arg(a, val, matcher, ValueType::EnvVariable, false);
                     } else {
                         self.check_for_help_and_version_str(val)?;
                         matcher.add_index_to(&a.id, self.cur_idx.get(), ValueType::EnvVariable);