From 908b7aeb44d1098d1270bd59fa5fcb87c2d7b2b3 Mon Sep 17 00:00:00 2001 From: Ivan Tham Date: Sun, 1 Mar 2020 17:07:37 +0800 Subject: [PATCH] Ambiguous suggetions for InferSubcommands Closes #1655 --- src/parse/errors.rs | 2 +- src/parse/features/suggestions.rs | 43 ++++++++++++++++++------------- src/parse/parser.rs | 22 +++++++++------- tests/subcommands.rs | 25 ++++++++++++++++++ 4 files changed, 63 insertions(+), 29 deletions(-) diff --git a/src/parse/errors.rs b/src/parse/errors.rs index 6291d48b..e48d1b11 100644 --- a/src/parse/errors.rs +++ b/src/parse/errors.rs @@ -628,7 +628,7 @@ impl Error { cause: format!("The subcommand '{}' wasn't recognized", s), message: format!( "{} The subcommand '{}' wasn't recognized\n\t\ - Did you mean '{}'?\n\n\ + Did you mean {}?\n\n\ If you believe you received this message in error, try \ re-running with '{} {} {}'\n\n\ {}\n\n\ diff --git a/src/parse/features/suggestions.rs b/src/parse/features/suggestions.rs index b63dd18f..117484cb 100644 --- a/src/parse/features/suggestions.rs +++ b/src/parse/features/suggestions.rs @@ -1,3 +1,5 @@ +use std::cmp::Ordering; + // Third Party #[cfg(feature = "suggestions")] use strsim; @@ -6,35 +8,32 @@ use strsim; use crate::build::App; use crate::output::fmt::Format; -/// Produces a string from a given list of possible values which is similar to -/// the passed in value `v` with a certain confidence. +/// Produces multiple strings from a given list of possible values which are similar +/// to the passed in value `v` within a certain confidence by least confidence. /// Thus in a list of possible values like ["foo", "bar"], the value "fop" will yield /// `Some("foo")`, whereas "blark" would yield `None`. #[cfg(feature = "suggestions")] -pub fn did_you_mean(v: &str, possible_values: I) -> Option +pub fn did_you_mean(v: &str, possible_values: I) -> Vec where T: AsRef, I: IntoIterator, { - let mut candidate: Option<(f64, String)> = None; - for pv in possible_values { - let confidence = strsim::jaro_winkler(v, pv.as_ref()); - if confidence > 0.8 && (candidate.is_none() || (candidate.as_ref().unwrap().0 < confidence)) - { - candidate = Some((confidence, pv.as_ref().to_owned())); - } - } - - candidate.map(|(_, candidate)| candidate) + let mut candidates: Vec<(f64, String)> = possible_values + .into_iter() + .map(|pv| (strsim::jaro_winkler(v, pv.as_ref()), pv.as_ref().to_owned())) + .filter(|(confidence, _pv)| *confidence > 0.8) + .collect(); + candidates.sort_by(|a, b| a.0.partial_cmp(&b.0).unwrap_or(Ordering::Equal)); + candidates.into_iter().map(|(_confidence, pv)| pv).collect() } #[cfg(not(feature = "suggestions"))] -pub fn did_you_mean(_: &str, _: I) -> Option +pub fn did_you_mean(_: &str, _: I) -> Vec where T: AsRef, I: IntoIterator, { - None + Vec::new() } /// Returns a suffix that can be empty, or is the standard 'did you mean' phrase @@ -47,7 +46,7 @@ where T: AsRef, I: IntoIterator, { - match did_you_mean(arg, longs) { + match did_you_mean(arg, longs).pop() { Some(ref candidate) => { let suffix = format!( "\n\tDid you mean {}{}?", @@ -62,7 +61,9 @@ where if let Some(ref candidate) = did_you_mean( arg, longs!(subcommand).map(|x| x.to_string_lossy().into_owned()), - ) { + ) + .pop() + { let suffix = format!( "\n\tDid you mean to put '{}{}' after the subcommand '{}'?", Format::Good("--"), @@ -83,7 +84,7 @@ where T: AsRef, I: IntoIterator, { - match did_you_mean(arg, values) { + match did_you_mean(arg, values).pop() { Some(ref candidate) => { let suffix = format!("\n\tDid you mean '{}'?", Format::Good(candidate)); (suffix, Some(candidate.to_owned())) @@ -102,6 +103,12 @@ mod test { assert_eq!(did_you_mean("tst", p_vals.iter()), Some("test")); } + #[test] + fn possible_values_match() { + let p_vals = ["test", "temp"]; + assert_eq!(did_you_mean("te", p_vals.iter()), Some("test")); + } + #[test] fn possible_values_nomatch() { let p_vals = ["test", "possible", "values"]; diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 89a39a08..bad6a62e 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -547,12 +547,13 @@ where || self.is_set(AS::AllowExternalSubcommands) || self.is_set(AS::InferSubcommands)) { - if let Some(cdate) = - suggestions::did_you_mean(&*arg_os.to_string_lossy(), sc_names!(self.app)) - { + let cands = + suggestions::did_you_mean(&*arg_os.to_string_lossy(), sc_names!(self.app)); + if !cands.is_empty() { + let cands: Vec<_> = cands.iter().map(|cand| format!("'{}'", cand)).collect(); return Err(ClapError::invalid_subcommand( arg_os.to_string_lossy().into_owned(), - cdate, + cands.join(" or "), self.app.bin_name.as_ref().unwrap_or(&self.app.name), &*Usage::new(self).create_usage_with_title(&[]), self.app.color(), @@ -608,8 +609,8 @@ where if self.is_new_arg(n, needs_val_of) || sc_match - || suggestions::did_you_mean(&n.to_string_lossy(), sc_names!(self.app)) - .is_some() + || !suggestions::did_you_mean(&n.to_string_lossy(), sc_names!(self.app)) + .is_empty() { debugln!("Parser::get_matches_with: Bumping the positional counter..."); pos_counter += 1; @@ -728,12 +729,13 @@ where self.app.color(), )); } else if !has_args || self.is_set(AS::InferSubcommands) && self.has_subcommands() { - if let Some(cdate) = - suggestions::did_you_mean(&*arg_os.to_string_lossy(), sc_names!(self.app)) - { + let cands = + suggestions::did_you_mean(&*arg_os.to_string_lossy(), sc_names!(self.app)); + if !cands.is_empty() { + let cands: Vec<_> = cands.iter().map(|cand| format!("'{}'", cand)).collect(); return Err(ClapError::invalid_subcommand( arg_os.to_string_lossy().into_owned(), - cdate, + cands.join(" or "), self.app.bin_name.as_ref().unwrap_or(&self.app.name), &*Usage::new(self).create_usage_with_title(&[]), self.app.color(), diff --git a/tests/subcommands.rs b/tests/subcommands.rs index fb455dd9..823a361e 100644 --- a/tests/subcommands.rs +++ b/tests/subcommands.rs @@ -39,6 +39,17 @@ USAGE: For more information try --help"; +#[cfg(feature = "suggestions")] +static DYM_SUBCMD_AMBIGUOUS: &str = "error: The subcommand 'te' wasn't recognized + Did you mean 'test' or 'temp'? + +If you believe you received this message in error, try re-running with 'dym -- te' + +USAGE: + dym [SUBCOMMAND] + +For more information try --help"; + #[cfg(feature = "suggestions")] static DYM_ARG: &str = "error: Found argument '--subcm' which wasn't expected, or isn't valid in this context @@ -136,6 +147,20 @@ fn subcmd_did_you_mean_output() { assert!(utils::compare_output(app, "dym subcm", DYM_SUBCMD, true)); } +#[test] +#[cfg(feature = "suggestions")] +fn subcmd_did_you_mean_output_ambiguous() { + let app = App::new("dym") + .subcommand(App::new("test")) + .subcommand(App::new("temp")); + assert!(utils::compare_output( + app, + "dym te", + DYM_SUBCMD_AMBIGUOUS, + true + )); +} + #[test] #[cfg(feature = "suggestions")] fn subcmd_did_you_mean_output_arg() {