Merge pull request #3765 from epage/refactor

fix(parser): Make behavior more consistent

src/builder/action.rs

@@ -0,0 +1,8 @@
+/// Behavior of arguments when they are encountered while parsing
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub(crate) enum Action {
+    StoreValue,
+    Flag,
+    Help,
+    Version,
+}

src/builder/arg.rs

@@ -18,6 +18,7 @@ use yaml_rust::Yaml;
 
 // Internal
 use crate::builder::usage_parser::UsageParser;
+use crate::builder::Action;
 use crate::builder::ArgPredicate;
 use crate::util::{Id, Key};
 use crate::PossibleValue;
@@ -63,6 +64,7 @@ pub struct Arg<'help> {
     pub(crate) name: &'help str,
     pub(crate) help: Option<&'help str>,
     pub(crate) long_help: Option<&'help str>,
+    pub(crate) action: Option<Action>,
     pub(crate) value_parser: Option<super::ValueParser>,
     pub(crate) blacklist: Vec<Id>,
     pub(crate) settings: ArgFlags,
@@ -4528,6 +4530,12 @@ impl<'help> Arg<'help> {
         self.is_set(ArgSettings::AllowInvalidUtf8)
     }
 
+    /// Behavior when parsing the argument
+    pub(crate) fn get_action(&self) -> &super::Action {
+        const DEFAULT: super::Action = super::Action::StoreValue;
+        self.action.as_ref().unwrap_or(&DEFAULT)
+    }
+
     /// Configured parser for argument values
     ///
     /// # Example

src/builder/command.rs

@@ -4115,6 +4115,10 @@ impl<'help> App<'help> {
             let mut pos_counter = 1;
             let self_override = self.is_set(AppSettings::AllArgsOverrideSelf);
             let hide_pv = self.is_set(AppSettings::HidePossibleValues);
+            let auto_help =
+                !self.is_set(AppSettings::NoAutoHelp) && !self.is_disable_help_flag_set();
+            let auto_version =
+                !self.is_set(AppSettings::NoAutoVersion) && !self.is_disable_version_flag_set();
             for a in self.args.args_mut() {
                 // Fill in the groups
                 for g in &a.groups {
@@ -4141,6 +4145,24 @@ impl<'help> App<'help> {
                     a.overrides.push(self_id);
                 }
                 a._build();
+                // HACK: Setting up action at this level while auto-help / disable help flag is
+                // required.  Otherwise, most of this won't be needed because when we can break
+                // compat, actions will reign supreme (default to `Store`)
+                if a.action.is_none() {
+                    if a.get_id() == "help" && auto_help {
+                        let action = super::Action::Help;
+                        a.action = Some(action);
+                    } else if a.get_id() == "version" && auto_version {
+                        let action = super::Action::Version;
+                        a.action = Some(action);
+                    } else if a.is_takes_value_set() {
+                        let action = super::Action::StoreValue;
+                        a.action = Some(action);
+                    } else {
+                        let action = super::Action::Flag;
+                        a.action = Some(action);
+                    }
+                }
                 if a.is_positional() && a.index.is_none() {
                     a.index = Some(pos_counter);
                     pos_counter += 1;

src/builder/mod.rs

@@ -3,6 +3,7 @@
 #[macro_use]
 mod macros;
 
+mod action;
 mod app_settings;
 mod arg;
 mod arg_group;
@@ -53,6 +54,7 @@ pub use command::App;
 #[cfg(feature = "regex")]
 pub use self::regex::RegexRef;
 
+pub(crate) use action::Action;
 pub(crate) use arg::display_arg_val;
 pub(crate) use arg_predicate::ArgPredicate;
 pub(crate) use value_parser::ValueParserInner;

src/parser/arg_matcher.rs

@@ -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);
 
@@ -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) {
@@ -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) {
@@ -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) {
@@ -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) {
@@ -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) {

src/parser/matches/matched_arg.rs

@@ -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()
@@ -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![]);
@@ -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;
@@ -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"),
-                ]
-            ]
-        )
-    }
 }