Merge pull request #4375 from epage/group2

fix(parser): Only add ArgGroup to ArgMatches for command-line

Author: epage

src/parser/arg_matcher.rs

@@ -152,23 +152,6 @@ impl ArgMatcher {
         ma.new_val_group();
     }
 
-    pub(crate) fn start_occurrence_of_arg(&mut self, arg: &Arg) {
-        let id = arg.get_id().clone();
-        debug!("ArgMatcher::start_occurrence_of_arg: id={:?}", id);
-        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(ValueSource::CommandLine);
-        ma.new_val_group();
-    }
-
-    pub(crate) fn start_occurrence_of_group(&mut self, id: Id) {
-        debug!("ArgMatcher::start_occurrence_of_group: id={:?}", id);
-        let ma = self.entry(id).or_insert(MatchedArg::new_group());
-        debug_assert_eq!(ma.type_id(), None);
-        ma.set_source(ValueSource::CommandLine);
-        ma.new_val_group();
-    }
-
     pub(crate) fn start_occurrence_of_external(&mut self, cmd: &crate::Command) {
         let id = Id::from_static_ref(Id::EXTERNAL);
         debug!("ArgMatcher::start_occurrence_of_external: id={:?}", id,);

src/parser/matches/matched_arg.rs

@@ -130,7 +130,7 @@ impl MatchedArg {
     }
 
     pub(crate) fn check_explicit(&self, predicate: &ArgPredicate) -> bool {
-        if self.source == Some(ValueSource::DefaultValue) {
+        if self.source.map(|s| !s.is_explicit()).unwrap_or(false) {
             return false;
         }
 

src/parser/matches/value_source.rs

@@ -9,3 +9,9 @@ pub enum ValueSource {
     /// Value was passed in on the command-line
     CommandLine,
 }
+
+impl ValueSource {
+    pub(crate) fn is_explicit(self) -> bool {
+        self != Self::DefaultValue
+    }
+}

src/parser/parser.rs

@@ -1078,15 +1078,6 @@ impl<'cmd> Parser<'cmd> {
             matcher.add_index_to(arg.get_id(), self.cur_idx.get());
         }
 
-        // Increment or create the group "args"
-        for group in self.cmd.groups_for_arg(arg.get_id()) {
-            matcher.add_val_to(
-                &group,
-                AnyValue::new(arg.get_id().clone()),
-                OsString::from(arg.get_id().as_str()),
-            );
-        }
-
         Ok(())
     }
 
@@ -1499,20 +1490,15 @@ impl<'cmd> Parser<'cmd> {
             self.remove_overrides(arg, matcher);
         }
         matcher.start_custom_arg(arg, source);
-        for group in self.cmd.groups_for_arg(arg.get_id()) {
-            matcher.start_custom_group(group, source);
-        }
-    }
-
-    /// Increase occurrence of specific argument and the grouped arg it's in.
-    fn start_occurrence_of_arg(&self, matcher: &mut ArgMatcher, arg: &Arg) {
-        // With each new occurrence, remove overrides from prior occurrences
-        self.remove_overrides(arg, matcher);
-
-        matcher.start_occurrence_of_arg(arg);
-        // Increment or create the group "args"
-        for group in self.cmd.groups_for_arg(arg.get_id()) {
-            matcher.start_occurrence_of_group(group);
+        if source.is_explicit() {
+            for group in self.cmd.groups_for_arg(arg.get_id()) {
+                matcher.start_custom_group(group.clone(), source);
+                matcher.add_val_to(
+                    &group,
+                    AnyValue::new(arg.get_id().clone()),
+                    OsString::from(arg.get_id().as_str()),
+                );
+            }
         }
     }
 }
@@ -1549,7 +1535,7 @@ impl<'cmd> Parser<'cmd> {
         // Add the arg to the matches to build a proper usage string
         if let Some((name, _)) = did_you_mean.as_ref() {
             if let Some(arg) = self.cmd.get_keymap().get(&name.as_ref()) {
-                self.start_occurrence_of_arg(matcher, arg);
+                self.start_custom_arg(matcher, arg, ValueSource::CommandLine);
             }
         }
 

tests/builder/groups.rs

@@ -73,9 +73,10 @@ fn group_single_value() {
 #[test]
 fn group_empty() {
     let res = Command::new("group")
+        .arg(arg!(-f --flag "some flag"))
         .arg(arg!(-c --color [color] "some option"))
         .arg(arg!(-n --hostname <name> "another option"))
-        .group(ArgGroup::new("grp").args(["hostname", "color"]))
+        .group(ArgGroup::new("grp").args(["hostname", "color", "flag"]))
         .try_get_matches_from(vec![""]);
     assert!(res.is_ok(), "{}", res.unwrap_err());
 
@@ -87,12 +88,13 @@ fn group_empty() {
 #[test]
 fn group_required_flags_empty() {
     let result = Command::new("group")
+        .arg(arg!(-f --flag "some flag"))
         .arg(arg!(-c --color "some option"))
         .arg(arg!(-n --hostname <name> "another option"))
         .group(
             ArgGroup::new("grp")
                 .required(true)
-                .args(["hostname", "color"]),
+                .args(["hostname", "color", "flag"]),
         )
         .try_get_matches_from(vec![""]);
     assert!(result.is_err());
@@ -103,9 +105,10 @@ fn group_required_flags_empty() {
 #[test]
 fn group_multi_value_single_arg() {
     let res = Command::new("group")
+        .arg(arg!(-f --flag "some flag"))
         .arg(arg!(-c --color <color> "some option").num_args(1..))
         .arg(arg!(-n --hostname <name> "another option"))
-        .group(ArgGroup::new("grp").args(["hostname", "color"]))
+        .group(ArgGroup::new("grp").args(["hostname", "color", "flag"]))
         .try_get_matches_from(vec!["", "-c", "blue", "red", "green"]);
     assert!(res.is_ok(), "{:?}", res.unwrap_err().kind());