From 8f4c26fc58eed3224fdb49754a2635e8e7926049 Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Sat, 26 Dec 2020 22:39:02 +0800 Subject: [PATCH 01/14] Minor redundant logic cleanup Fix clippy dot fix --- src/output/usage.rs | 4 ++- src/parse/parser.rs | 68 +++++++++++++++++++-------------------------- 2 files changed, 31 insertions(+), 41 deletions(-) diff --git a/src/output/usage.rs b/src/output/usage.rs index 830b221b6e6..d6f51b8d41f 100644 --- a/src/output/usage.rs +++ b/src/output/usage.rs @@ -365,6 +365,8 @@ impl<'help, 'app, 'parser> Usage<'help, 'app, 'parser> { // `incl_last`: should we include args that are Arg::Last? (i.e. `prog [foo] -- [last]). We // can't do that for required usages being built for subcommands because it would look like: // `prog [foo] -- [last] ` which is totally wrong. + // TODO: remove the allow clippy when we update the compiler version. + #[allow(clippy::needless_collect)] pub(crate) fn get_required_usage_from( &self, incls: &[Id], @@ -419,7 +421,7 @@ impl<'help, 'app, 'parser> Usage<'help, 'app, 'parser> { for p in pmap.values() { debug!("Usage::get_required_usage_from:iter:{:?}", p.id); - if args_in_groups.is_empty() || !args_in_groups.contains(&p.id) { + if !args_in_groups.contains(&p.id) { ret_val.push(p.to_string()); } } diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 39df265b802..c8e8cc84643 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -356,25 +356,25 @@ impl<'help, 'app> Parser<'help, 'app> { // Has the user already passed '--'? Meaning only positional args follow if !self.is_set(AS::TrailingValues) { - // Does the arg match a subcommand name, or any of its aliases (if defined) - match needs_val_of { - ParseResult::Opt(_) | ParseResult::Pos(_) - if !self.is_set(AS::SubcommandPrecedenceOverArg) => {} - _ => { - let sc_name = self.possible_subcommand(&arg_os); - debug!( - "Parser::get_matches_with: possible_sc={:?}, sc={:?}", - sc_name.is_some(), - sc_name - ); - if let Some(sc_name) = sc_name { - if sc_name == "help" && !self.is_set(AS::NoAutoHelp) { - self.parse_help_subcommand(remaining_args)?; - } + let can_be_subcommand = if self.is_set(AS::SubcommandPrecedenceOverArg) { + true + } else { + match needs_val_of { + ParseResult::Opt(_) | ParseResult::Pos(_) => false, + _ => true, + } + }; - subcmd_name = Some(sc_name.to_owned()); - break; + if can_be_subcommand { + // Does the arg match a subcommand name, or any of its aliases (if defined) + let sc_name = self.possible_subcommand(&arg_os); + debug!("Parser::get_matches_with: sc={:?}", sc_name); + if let Some(sc_name) = sc_name { + if sc_name == "help" && !self.is_set(AS::NoAutoHelp) { + self.parse_help_subcommand(remaining_args)?; } + subcmd_name = Some(sc_name.to_owned()); + break; } } @@ -477,31 +477,20 @@ impl<'help, 'app> Parser<'help, 'app> { if low_index_mults || missing_pos { if let Some(n) = remaining_args.get(0) { - needs_val_of = match needs_val_of { - ParseResult::ValuesDone => ParseResult::ValuesDone, - _ => { - if let Some(p) = self - .app - .get_positionals() - .find(|p| p.index == Some(pos_counter as u64)) - { - ParseResult::Pos(p.id.clone()) - } else { - ParseResult::ValuesDone - } - } + needs_val_of = if let Some(p) = self + .app + .get_positionals() + .find(|p| p.index == Some(pos_counter as u64)) + { + ParseResult::Pos(p.id.clone()) + } else { + ParseResult::ValuesDone }; + // If the arg doesn't looks like a new_arg and it's not a + // subcommand, don't bump the position counter. let n = ArgStr::new(n); - let sc_match = { self.possible_subcommand(&n).is_some() }; - - if self.is_new_arg(&n, &needs_val_of) - || sc_match - || !suggestions::did_you_mean( - &n.to_string_lossy(), - self.app.all_subcommand_names(), - ) - .is_empty() + if self.is_new_arg(&n, &needs_val_of) || self.possible_subcommand(&n).is_some() { debug!("Parser::get_matches_with: Bumping the positional counter..."); pos_counter += 1; @@ -548,7 +537,6 @@ impl<'help, 'app> Parser<'help, 'app> { matcher.inc_occurrence_of(&grp); } - self.app.settings.set(AS::ValidArgFound); // Only increment the positional counter if it doesn't allow multiples if !p.settings.is_set(ArgSettings::MultipleValues) { pos_counter += 1; From c6bcfe819ecf479ed4b4cec4389f37ab02f9cbc1 Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Tue, 29 Dec 2020 17:38:43 +0800 Subject: [PATCH 02/14] Clearer error processing --- src/parse/parser.rs | 69 ++++++++++++++++++++++----------------------- 1 file changed, 34 insertions(+), 35 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index c8e8cc84643..e3ad7ad04fa 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -592,41 +592,40 @@ impl<'help, 'app> Parser<'help, 'app> { self.app.color(), )); } - } else { - let cands = suggestions::did_you_mean( - &arg_os.to_string_lossy(), - self.app.all_subcommand_names(), - ); - // If the argument looks like a subcommand. - if !cands.is_empty() { - let cands: Vec<_> = - cands.iter().map(|cand| format!("'{}'", cand)).collect(); - return Err(ClapError::invalid_subcommand( - arg_os.to_string_lossy().to_string(), - cands.join(" or "), - self.app - .bin_name - .as_ref() - .unwrap_or(&self.app.name) - .to_string(), - Usage::new(self).create_usage_with_title(&[]), - self.app.color(), - )); - } - // If the argument must be a subcommand. - if !self.has_args() - || self.is_set(AS::InferSubcommands) && self.has_subcommands() - { - return Err(ClapError::unrecognized_subcommand( - arg_os.to_string_lossy().to_string(), - self.app - .bin_name - .as_ref() - .unwrap_or(&self.app.name) - .to_string(), - self.app.color(), - )); - } + } + let cands = suggestions::did_you_mean( + &arg_os.to_string_lossy(), + self.app.all_subcommand_names(), + ); + // If the argument looks like a subcommand. + if !cands.is_empty() { + let cands: Vec<_> = + cands.iter().map(|cand| format!("'{}'", cand)).collect(); + return Err(ClapError::invalid_subcommand( + arg_os.to_string_lossy().to_string(), + cands.join(" or "), + self.app + .bin_name + .as_ref() + .unwrap_or(&self.app.name) + .to_string(), + Usage::new(self).create_usage_with_title(&[]), + self.app.color(), + )); + } + // If the argument must be a subcommand. + if !self.has_args() + || self.is_set(AS::InferSubcommands) && self.has_subcommands() + { + return Err(ClapError::unrecognized_subcommand( + arg_os.to_string_lossy().to_string(), + self.app + .bin_name + .as_ref() + .unwrap_or(&self.app.name) + .to_string(), + self.app.color(), + )); } return Err(ClapError::unknown_argument( arg_os.to_string_lossy().to_string(), From 47eee8ab2fb396e0900aabecd6e682763efc5b5e Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Tue, 29 Dec 2020 17:46:29 +0800 Subject: [PATCH 03/14] Acc a request --- src/parse/parser.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index e3ad7ad04fa..b9197c3c827 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -511,8 +511,7 @@ impl<'help, 'app> Parser<'help, 'app> { if let Some(p) = self .app .args - .args() - .find(|a| a.is_positional() && a.index == Some(pos_counter as u64)) + .get(&(pos_counter as u64)) { if p.is_set(ArgSettings::Last) && !self.is_set(AS::TrailingValues) { return Err(ClapError::unknown_argument( @@ -599,8 +598,7 @@ impl<'help, 'app> Parser<'help, 'app> { ); // If the argument looks like a subcommand. if !cands.is_empty() { - let cands: Vec<_> = - cands.iter().map(|cand| format!("'{}'", cand)).collect(); + let cands: Vec<_> = cands.iter().map(|cand| format!("'{}'", cand)).collect(); return Err(ClapError::invalid_subcommand( arg_os.to_string_lossy().to_string(), cands.join(" or "), @@ -614,9 +612,7 @@ impl<'help, 'app> Parser<'help, 'app> { )); } // If the argument must be a subcommand. - if !self.has_args() - || self.is_set(AS::InferSubcommands) && self.has_subcommands() - { + if !self.has_args() || self.is_set(AS::InferSubcommands) && self.has_subcommands() { return Err(ClapError::unrecognized_subcommand( arg_os.to_string_lossy().to_string(), self.app From 1b63516e3bd6c024f349789112faa90d96a95ea9 Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Tue, 29 Dec 2020 18:10:05 +0800 Subject: [PATCH 04/14] Flow reordering --- src/parse/parser.rs | 57 ++++++++++++++++++--------------------------- 1 file changed, 23 insertions(+), 34 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index b9197c3c827..cde453269e5 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -345,17 +345,15 @@ impl<'help, 'app> Parser<'help, 'app> { arg_os.as_raw_bytes() ); - // Is this a new argument, or a value for previous option? - let starts_new_arg = self.is_new_arg(&arg_os, &needs_val_of); - - if !self.is_set(AS::TrailingValues) && arg_os == "--" && starts_new_arg { - debug!("Parser::get_matches_with: setting TrailingVals=true"); - self.set(AS::TrailingValues); - continue; - } - // Has the user already passed '--'? Meaning only positional args follow if !self.is_set(AS::TrailingValues) { + // Is this a new argument, or a value for previous option? + let starts_new_arg = self.is_new_arg(&arg_os, &needs_val_of); + if arg_os == "--" && starts_new_arg { + debug!("Parser::get_matches_with: setting TrailingVals=true"); + self.set(AS::TrailingValues); + continue; + } let can_be_subcommand = if self.is_set(AS::SubcommandPrecedenceOverArg) { true } else { @@ -508,11 +506,7 @@ impl<'help, 'app> Parser<'help, 'app> { pos_counter = positional_count; } - if let Some(p) = self - .app - .args - .get(&(pos_counter as u64)) - { + if let Some(p) = self.app.args.get(&(pos_counter as u64)) { if p.is_set(ArgSettings::Last) && !self.is_set(AS::TrailingValues) { return Err(ClapError::unknown_argument( arg_os.to_string_lossy().to_string(), @@ -1236,25 +1230,19 @@ impl<'help, 'app> Parser<'help, 'app> { ); if !(self.is_set(AS::TrailingValues) && self.is_set(AS::DontDelimitTrailingValues)) { if let Some(delim) = arg.val_delim { - if val.is_empty() { - Ok(self.add_single_val_to_arg(arg, val, matcher, ty)?) - } else { - let mut iret = ParseResult::ValuesDone; - for v in val.split(delim) { - iret = self.add_single_val_to_arg(arg, &v, matcher, ty)?; - } - // If there was a delimiter used, we're not looking for more values - if val.contains_char(delim) || arg.is_set(ArgSettings::RequireDelimiter) { - iret = ParseResult::ValuesDone; - } - Ok(iret) + let mut iret = ParseResult::ValuesDone; + for v in val.split(delim) { + iret = self.add_single_val_to_arg(arg, &v, matcher, ty)?; } - } else { - self.add_single_val_to_arg(arg, val, matcher, ty) + // 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) { + iret = ParseResult::ValuesDone; + } + return Ok(iret); } - } else { - self.add_single_val_to_arg(arg, val, matcher, ty) } + self.add_single_val_to_arg(arg, val, matcher, ty) } fn add_single_val_to_arg( @@ -1280,14 +1268,15 @@ impl<'help, 'app> Parser<'help, 'app> { matcher.add_index_to(&arg.id, self.cur_idx.get(), ty); // Increment or create the group "args" - for grp in self.app.groups_for_arg(&arg.id) { - matcher.add_val_to(&grp, v.to_os_string(), ty); + for group in self.app.groups_for_arg(&arg.id) { + matcher.add_val_to(&group, v.to_os_string(), ty); } if matcher.needs_more_vals(arg) { - return Ok(ParseResult::Opt(arg.id.clone())); + Ok(ParseResult::Opt(arg.id.clone())) + } else { + Ok(ParseResult::ValuesDone) } - Ok(ParseResult::ValuesDone) } fn parse_flag(&self, flag: &Arg<'help>, matcher: &mut ArgMatcher) -> ClapResult { From c39dcf2ecf80898ae25a38c59b34e7138bf4728b Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Tue, 29 Dec 2020 19:05:56 +0800 Subject: [PATCH 05/14] Remove redundant ClapResult in some functions --- src/parse/parser.rs | 62 +++++++++++++++++------------------------- src/parse/validator.rs | 2 +- 2 files changed, 26 insertions(+), 38 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index cde453269e5..2178408aa8d 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -357,10 +357,7 @@ impl<'help, 'app> Parser<'help, 'app> { let can_be_subcommand = if self.is_set(AS::SubcommandPrecedenceOverArg) { true } else { - match needs_val_of { - ParseResult::Opt(_) | ParseResult::Pos(_) => false, - _ => true, - } + !matches!(needs_val_of, ParseResult::Opt(_) | ParseResult::Pos(_)) }; if can_be_subcommand { @@ -439,7 +436,7 @@ impl<'help, 'app> Parser<'help, 'app> { &arg_os, matcher, ValueType::CommandLine, - )?; + ); // get the next value from the iterator continue; } @@ -523,7 +520,7 @@ impl<'help, 'app> Parser<'help, 'app> { } self.seen.push(p.id.clone()); - self.add_val_to_arg(p, &arg_os, matcher, ValueType::CommandLine)?; + self.add_val_to_arg(p, &arg_os, matcher, ValueType::CommandLine); matcher.inc_occurrence_of(&p.id); for grp in self.app.groups_for_arg(&p.id) { @@ -1034,7 +1031,7 @@ impl<'help, 'app> Parser<'help, 'app> { return Ok(self.parse_opt(&val, opt, val.is_some(), matcher)?); } self.check_for_help_and_version_str(&arg)?; - self.parse_flag(opt, matcher)?; + self.parse_flag(opt, matcher); return Ok(ParseResult::Flag); } else if let Some(sc_name) = self.possible_long_flag_subcommand(&arg) { @@ -1093,7 +1090,7 @@ impl<'help, 'app> Parser<'help, 'app> { self.seen.push(opt.id.clone()); if !opt.is_set(ArgSettings::TakesValue) { self.check_for_help_and_version_char(c)?; - ret = self.parse_flag(opt, matcher)?; + ret = self.parse_flag(opt, matcher); continue; } @@ -1171,7 +1168,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); } else if needs_eq && !(empty_vals || min_vals_zero) { debug!("None, but requires equals...Error"); return Err(ClapError::empty_value( @@ -1188,7 +1185,7 @@ impl<'help, 'app> Parser<'help, 'app> { "Parser::parse_opt: adding value from default_missing_values; val = {:?}", val ); - self.add_val_to_arg(opt, &ArgStr::new(val), matcher, ValueType::CommandLine)?; + self.add_val_to_arg(opt, &ArgStr::new(val), matcher, ValueType::CommandLine); } }; } else { @@ -1221,7 +1218,7 @@ impl<'help, 'app> Parser<'help, 'app> { val: &ArgStr, matcher: &mut ArgMatcher, ty: ValueType, - ) -> ClapResult { + ) -> ParseResult { debug!("Parser::add_val_to_arg; arg={}, val={:?}", arg.name, val); debug!( "Parser::add_val_to_arg; trailing_vals={:?}, DontDelimTrailingVals={:?}", @@ -1232,14 +1229,14 @@ impl<'help, 'app> Parser<'help, 'app> { if let Some(delim) = arg.val_delim { let mut iret = ParseResult::ValuesDone; for v in val.split(delim) { - iret = self.add_single_val_to_arg(arg, &v, matcher, ty)?; + iret = self.add_single_val_to_arg(arg, &v, matcher, ty); } // 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) { iret = ParseResult::ValuesDone; } - return Ok(iret); + return iret; } } self.add_single_val_to_arg(arg, val, matcher, ty) @@ -1251,7 +1248,7 @@ impl<'help, 'app> Parser<'help, 'app> { v: &ArgStr, matcher: &mut ArgMatcher, ty: ValueType, - ) -> ClapResult { + ) -> ParseResult { debug!("Parser::add_single_val_to_arg: adding val...{:?}", v); // update the current index because each value is a distinct index to clap @@ -1260,7 +1257,7 @@ impl<'help, 'app> Parser<'help, 'app> { // @TODO @docs @p4 docs should probably note that terminator doesn't get an index if let Some(t) = arg.terminator { if t == v { - return Ok(ParseResult::ValuesDone); + return ParseResult::ValuesDone; } } @@ -1273,13 +1270,13 @@ impl<'help, 'app> Parser<'help, 'app> { } if matcher.needs_more_vals(arg) { - Ok(ParseResult::Opt(arg.id.clone())) + ParseResult::Opt(arg.id.clone()) } else { - Ok(ParseResult::ValuesDone) + ParseResult::ValuesDone } } - fn parse_flag(&self, flag: &Arg<'help>, matcher: &mut ArgMatcher) -> ClapResult { + fn parse_flag(&self, flag: &Arg<'help>, matcher: &mut ArgMatcher) -> ParseResult { debug!("Parser::parse_flag"); matcher.inc_occurrence_of(&flag.id); @@ -1289,7 +1286,7 @@ impl<'help, 'app> Parser<'help, 'app> { matcher.inc_occurrence_of(&grp); } - Ok(ParseResult::Flag) + ParseResult::Flag } fn remove_overrides(&mut self, matcher: &mut ArgMatcher) { @@ -1362,28 +1359,21 @@ impl<'help, 'app> Parser<'help, 'app> { } } - pub(crate) fn add_defaults(&mut self, matcher: &mut ArgMatcher) -> ClapResult<()> { + pub(crate) fn add_defaults(&mut self, matcher: &mut ArgMatcher) { debug!("Parser::add_defaults"); for o in self.app.get_opts() { debug!("Parser::add_defaults:iter:{}:", o.name); - self.add_value(o, matcher, ValueType::DefaultValue)?; + self.add_value(o, matcher, ValueType::DefaultValue); } for p in self.app.get_positionals() { debug!("Parser::add_defaults:iter:{}:", p.name); - self.add_value(p, matcher, ValueType::DefaultValue)?; + self.add_value(p, matcher, ValueType::DefaultValue); } - - Ok(()) } - fn add_value( - &self, - arg: &Arg<'help>, - matcher: &mut ArgMatcher, - ty: ValueType, - ) -> ClapResult<()> { + fn add_value(&self, arg: &Arg<'help>, matcher: &mut ArgMatcher, ty: ValueType) { if !arg.default_vals_ifs.is_empty() { debug!("Parser::add_value: has conditional defaults"); @@ -1401,7 +1391,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); done = true; break; } @@ -1409,7 +1399,7 @@ impl<'help, 'app> Parser<'help, 'app> { } if done { - return Ok(()); + return; } } else { debug!("Parser::add_value: doesn't have conditional defaults"); @@ -1424,7 +1414,7 @@ impl<'help, 'app> Parser<'help, 'app> { 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)?; + self.add_val_to_arg(arg, &ArgStr::new(val), matcher, ty); } } } else { @@ -1448,7 +1438,7 @@ impl<'help, 'app> Parser<'help, 'app> { arg.name ); for val in &arg.default_missing_vals { - self.add_val_to_arg(arg, &ArgStr::new(val), matcher, ty)?; + self.add_val_to_arg(arg, &ArgStr::new(val), matcher, ty); } } None => { @@ -1468,8 +1458,6 @@ impl<'help, 'app> Parser<'help, 'app> { // do nothing } - - Ok(()) } pub(crate) fn add_env(&mut self, matcher: &mut ArgMatcher) -> ClapResult<()> { @@ -1479,7 +1467,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); } else { self.check_for_help_and_version_str(val)?; matcher.add_index_to(&a.id, self.cur_idx.get(), ValueType::EnvVariable); diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 2fef38aa232..a9fb3aefbb1 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -32,7 +32,7 @@ impl<'help, 'app, 'parser> Validator<'help, 'app, 'parser> { debug!("Validator::validate"); let mut reqs_validated = false; self.p.add_env(matcher)?; - self.p.add_defaults(matcher)?; + self.p.add_defaults(matcher); if let ParseResult::Opt(a) = needs_val_of { debug!("Validator::validate: needs_val_of={:?}", a); self.validate_required(matcher)?; From be314d9ac0f5588036f1f5c8550b97ba445a4b8c Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Tue, 29 Dec 2020 19:15:55 +0800 Subject: [PATCH 06/14] Flow optimize --- src/parse/parser.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 2178408aa8d..1d011259c02 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -1376,8 +1376,6 @@ impl<'help, 'app> Parser<'help, 'app> { fn add_value(&self, arg: &Arg<'help>, matcher: &mut ArgMatcher, ty: ValueType) { if !arg.default_vals_ifs.is_empty() { debug!("Parser::add_value: has conditional defaults"); - - let mut done = false; if matcher.get(&arg.id).is_none() { for (id, val, default) in arg.default_vals_ifs.values() { let add = if let Some(a) = matcher.get(&id) { @@ -1392,15 +1390,10 @@ impl<'help, 'app> Parser<'help, 'app> { if add { self.add_val_to_arg(arg, &ArgStr::new(default), matcher, ty); - done = true; - break; + return; } } } - - if done { - return; - } } else { debug!("Parser::add_value: doesn't have conditional defaults"); } From 336f926ec991337fc1d0cca75e0067aa2c9e54df Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Tue, 29 Dec 2020 21:09:56 +0800 Subject: [PATCH 07/14] Integrate workflow --- src/parse/parser.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 1d011259c02..3062c48bf93 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -347,13 +347,6 @@ impl<'help, 'app> Parser<'help, 'app> { // Has the user already passed '--'? Meaning only positional args follow if !self.is_set(AS::TrailingValues) { - // Is this a new argument, or a value for previous option? - let starts_new_arg = self.is_new_arg(&arg_os, &needs_val_of); - if arg_os == "--" && starts_new_arg { - debug!("Parser::get_matches_with: setting TrailingVals=true"); - self.set(AS::TrailingValues); - continue; - } let can_be_subcommand = if self.is_set(AS::SubcommandPrecedenceOverArg) { true } else { @@ -373,8 +366,14 @@ impl<'help, 'app> Parser<'help, 'app> { } } + // Is this a new argument, or a value for previous option? + let starts_new_arg = self.is_new_arg(&arg_os, &needs_val_of); if starts_new_arg { - if arg_os.starts_with("--") { + if arg_os == "--" { + debug!("Parser::get_matches_with: setting TrailingVals=true"); + self.set(AS::TrailingValues); + continue; + } else if arg_os.starts_with("--") { needs_val_of = self.parse_long_arg(matcher, &arg_os, remaining_args)?; debug!( "Parser::get_matches_with: After parse_long_arg {:?}", From 9cf007378b9d6408c60f2c9fc00979ccc63ab937 Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Tue, 29 Dec 2020 23:15:50 +0800 Subject: [PATCH 08/14] Branch simpification --- src/parse/parser.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 3062c48bf93..b4473936369 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -367,8 +367,7 @@ impl<'help, 'app> Parser<'help, 'app> { } // Is this a new argument, or a value for previous option? - let starts_new_arg = self.is_new_arg(&arg_os, &needs_val_of); - if starts_new_arg { + if self.is_new_arg(&arg_os, &needs_val_of) { if arg_os == "--" { debug!("Parser::get_matches_with: setting TrailingVals=true"); self.set(AS::TrailingValues); @@ -512,9 +511,7 @@ impl<'help, 'app> Parser<'help, 'app> { )); } - if !self.is_set(AS::TrailingValues) - && (self.is_set(AS::TrailingVarArg) && pos_counter == positional_count) - { + if self.is_set(AS::TrailingVarArg) && pos_counter == positional_count { self.app.settings.set(AS::TrailingValues); } @@ -1003,7 +1000,7 @@ impl<'help, 'app> Parser<'help, 'app> { // maybe here lifetime should be 'a debug!("Parser::parse_long_arg"); - // Update the curent index + // Update the current index self.cur_idx.set(self.cur_idx.get() + 1); debug!("Parser::parse_long_arg: Does it contain '='..."); From 5774eb2c52a9afcf587e964be559d695cea18fcf Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Tue, 29 Dec 2020 23:49:41 +0800 Subject: [PATCH 09/14] Simplify possible subcommand --- src/parse/parser.rs | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index b4473936369..66bc25f4b9b 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -667,30 +667,27 @@ impl<'help, 'app> Parser<'help, 'app> { fn possible_subcommand(&self, arg_os: &ArgStr) -> Option<&str> { debug!("Parser::possible_subcommand: arg={:?}", arg_os); - if self.is_set(AS::ArgsNegateSubcommands) && self.is_set(AS::ValidArgFound) { - return None; - } + if !(self.is_set(AS::ArgsNegateSubcommands) && self.is_set(AS::ValidArgFound)) { + if self.is_set(AS::InferSubcommands) { + // For subcommand `test`, we accepts it's prefix: `t`, `te`, + // `tes` and `test`. + let v = self + .app + .all_subcommand_names() + .filter(|s| arg_os.is_prefix_of(s)) + .collect::>(); - if self.is_set(AS::InferSubcommands) { - let v = self - .app - .all_subcommand_names() - .filter(|s| arg_os.is_prefix_of(s)) - .collect::>(); + if v.len() == 1 { + return Some(v[0]); + } - if v.len() == 1 { - return Some(v[0]); + // If there is any ambiguity, fallback to non-infer subcommand + // search. } - - for sc in &v { - if sc == arg_os { - return Some(sc); - } + if let Some(sc) = self.app.find_subcommand(arg_os) { + return Some(&sc.name); } - } else if let Some(sc) = self.app.find_subcommand(arg_os) { - return Some(&sc.name); } - None } From 944fc759c9a26966a3e25c76a8761532b02f939c Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Wed, 30 Dec 2020 00:01:29 +0800 Subject: [PATCH 10/14] Optimize branch, more comments --- src/parse/parser.rs | 40 +++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 66bc25f4b9b..f8e0b52f629 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -1001,15 +1001,14 @@ impl<'help, 'app> Parser<'help, 'app> { self.cur_idx.set(self.cur_idx.get() + 1); debug!("Parser::parse_long_arg: Does it contain '='..."); - let matches; + let long_arg = full_arg.trim_start_matches(b'-'); let (arg, val) = if full_arg.contains_byte(b'=') { - matches = full_arg.trim_start_matches(b'-'); - let (p0, p1) = matches.split_at_byte(b'='); + let (p0, p1) = long_arg.split_at_byte(b'='); debug!("Yes '{:?}'", p1); (p0, Some(p1)) } else { debug!("No"); - (full_arg.trim_start_matches(b'-'), None) + (long_arg, None) }; if let Some(opt) = self.app.args.get(&arg.to_os_string()) { debug!( @@ -1017,29 +1016,27 @@ impl<'help, 'app> Parser<'help, 'app> { opt.to_string() ); self.app.settings.set(AS::ValidArgFound); - self.seen.push(opt.id.clone()); - if opt.is_set(ArgSettings::TakesValue) { - return Ok(self.parse_opt(&val, opt, val.is_some(), matcher)?); + Ok(self.parse_opt(&val, opt, val.is_some(), matcher)?) + } else { + self.check_for_help_and_version_str(&arg)?; + self.parse_flag(opt, matcher); + Ok(ParseResult::Flag) } - self.check_for_help_and_version_str(&arg)?; - self.parse_flag(opt, matcher); - - return Ok(ParseResult::Flag); } else if let Some(sc_name) = self.possible_long_flag_subcommand(&arg) { - return Ok(ParseResult::FlagSubCommand(sc_name.to_string())); + Ok(ParseResult::FlagSubCommand(sc_name.to_string())) } else if self.is_set(AS::AllowLeadingHyphen) { - return Ok(ParseResult::MaybeHyphenValue); + Ok(ParseResult::MaybeHyphenValue) + } else { + debug!("Parser::parse_long_arg: Didn't match anything"); + let remaining_args: Vec<_> = remaining_args + .iter() + .map(|x| x.to_str().expect(INVALID_UTF8)) + .collect(); + self.did_you_mean_error(arg.to_str().expect(INVALID_UTF8), matcher, &remaining_args) + .map(|_| ParseResult::NotFound) } - - debug!("Parser::parse_long_arg: Didn't match anything"); - let remaining_args: Vec<_> = remaining_args - .iter() - .map(|x| x.to_str().expect(INVALID_UTF8)) - .collect(); - self.did_you_mean_error(arg.to_str().expect(INVALID_UTF8), matcher, &remaining_args) - .map(|_| ParseResult::NotFound) } fn parse_short_arg( @@ -1137,6 +1134,7 @@ impl<'help, 'app> Parser<'help, 'app> { ) -> ClapResult { debug!("Parser::parse_opt; opt={}, val={:?}", opt.name, val); debug!("Parser::parse_opt; opt.settings={:?}", opt.settings); + // has_eq: --flag=value let mut has_eq = false; let no_val = val.is_none(); let empty_vals = opt.is_set(ArgSettings::AllowEmptyValues); From c8e669c690db574d306d498a4057ddb25573196d Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Wed, 30 Dec 2020 00:39:20 +0800 Subject: [PATCH 11/14] Eliminate unreachable branch comment fix Apply fmt --- src/parse/parser.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index f8e0b52f629..4e05473fdfb 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -892,7 +892,7 @@ impl<'help, 'app> Parser<'help, 'app> { { let mut p = Parser::new(sc); // HACK: maintain indexes between parsers - // FlagSubCommand short arg needs to revist the current short args, but skip the subcommand itself + // FlagSubCommand short arg needs to revisit the current short args, but skip the subcommand itself if keep_state { p.cur_idx.set(self.cur_idx.get()); p.skip_idxs = self.skip_idxs; @@ -1034,8 +1034,11 @@ impl<'help, 'app> Parser<'help, 'app> { .iter() .map(|x| x.to_str().expect(INVALID_UTF8)) .collect(); - self.did_you_mean_error(arg.to_str().expect(INVALID_UTF8), matcher, &remaining_args) - .map(|_| ParseResult::NotFound) + Err(self.did_you_mean_error( + arg.to_str().expect(INVALID_UTF8), + matcher, + &remaining_args, + )) } } @@ -1470,7 +1473,7 @@ impl<'help, 'app> Parser<'help, 'app> { arg: &str, matcher: &mut ArgMatcher, remaining_args: &[&str], - ) -> ClapResult<()> { + ) -> ClapError { debug!("Parser::did_you_mean_error: arg={}", arg); // Didn't match a flag or option let longs = self @@ -1511,12 +1514,12 @@ impl<'help, 'app> Parser<'help, 'app> { .cloned() .collect(); - Err(ClapError::unknown_argument( + ClapError::unknown_argument( format!("--{}", arg), did_you_mean, Usage::new(self).create_usage_with_title(&*used), self.app.color(), - )) + ) } pub(crate) fn write_help_err(&self) -> ClapResult { From cf4881182abf45ad755fa8eb14cca044948d87c2 Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Wed, 30 Dec 2020 01:06:27 +0800 Subject: [PATCH 12/14] Add comments for some internal app settings --- src/build/app/settings.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/build/app/settings.rs b/src/build/app/settings.rs index 45dc922bce9..749f5467cf8 100644 --- a/src/build/app/settings.rs +++ b/src/build/app/settings.rs @@ -1046,12 +1046,16 @@ pub enum AppSettings { TrailingValues, #[doc(hidden)] + /// If the app is already built, used for caching. Built, #[doc(hidden)] + /// If the app's bin name is already built, used for caching. BinNameBuilt, #[doc(hidden)] + /// This is paired with `ArgsNegateSubcommands`. Used to determine if we + /// already met any valid arg(then we shouldn't expect subcommands after it). ValidArgFound, #[doc(hidden)] From 45753d552b166f81a63924baa144ee3f83ca223b Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Wed, 30 Dec 2020 02:13:36 +0800 Subject: [PATCH 13/14] Resolve a TODO --- src/parse/arg_matcher.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/parse/arg_matcher.rs b/src/parse/arg_matcher.rs index e4a520be6c5..88fca8c4211 100644 --- a/src/parse/arg_matcher.rs +++ b/src/parse/arg_matcher.rs @@ -130,7 +130,10 @@ impl ArgMatcher { pub(crate) fn add_val_to(&mut self, arg: &Id, val: OsString, ty: ValueType) { let ma = self.entry(arg).or_insert(MatchedArg { - occurs: 0, // @TODO @question Shouldn't this be 1 if we're already adding a value to this arg? + // We will manually inc occurrences later(for flexibility under + // specific circumstances, like only add one occurrence for flag + // when we met: `--flag=one,two`). + occurs: 0, ty, indices: Vec::with_capacity(1), vals: Vec::with_capacity(1), From 818f62bd5c1c3b07b148d782f454984511ae4d08 Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Wed, 30 Dec 2020 02:18:53 +0800 Subject: [PATCH 14/14] Unused clippy allow --- src/parse/parser.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 4e05473fdfb..107c01312de 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -283,7 +283,6 @@ impl<'help, 'app> Parser<'help, 'app> { true } - #[allow(clippy::blocks_in_if_conditions)] // Does all the initializing and prepares the parser pub(crate) fn _build(&mut self) { debug!("Parser::_build");