Repository: pacak/bpaf
Pull request #174 (2023-03-02 18:10:37)

Various error messages improvements
Fixes https://github.com/pacak/bpaf/issues/151
Fixes https://github.com/pacak/bpaf/issues/154


Patch:
diff --git a/examples/very_custom_usage.rs b/examples/very_custom_usage.rs
index a45e60d..695004c 100644
--- a/examples/very_custom_usage.rs
+++ b/examples/very_custom_usage.rs
@@ -20,6 +20,7 @@ enum Opts {
 // defines the parser we are going to use later
 #[derive(Debug, Clone, Bpaf)]
 #[bpaf(options)]
+#[allow(dead_code)]
 struct Cmd {
     opt: bool,
 }
diff --git a/src/meta_youmean.rs b/src/meta_youmean.rs
index afc41fe..764154b 100644
--- a/src/meta_youmean.rs
+++ b/src/meta_youmean.rs
@@ -34,12 +34,24 @@ pub(crate) fn suggest(args: &Args, meta: &Meta) -> Result<(), Error> {
     }
 
     let mut variants = Vec::new();
-    inner(arg, meta, &mut variants);
+    collect_suggestions(arg, meta, &mut variants, true);
 
     variants.sort_by(|a, b| b.0.cmp(&a.0));
 
-    if let Some((l, best)) = variants.pop() {
-        if l > 0 {
+    if let Some((l, (actual, expected))) = variants.pop() {
+        if let (0, I::Nested(cmd)) = (l, expected) {
+            let best = format!(
+                "{:?} is not valid in this context, did you mean to pass it to command \"{}\"?",
+                actual,
+                w_flag!(cmd),
+            );
+            return Err(Error::Stderr(best));
+        } else if l > 0 {
+            let best = format!(
+                "No such {:?}, did you mean `{}`?",
+                actual,
+                w_flag!(expected)
+            );
             return Err(Error::Stderr(best));
         }
     }
@@ -50,8 +62,10 @@ pub(crate) fn suggest(args: &Args, meta: &Meta) -> Result<(), Error> {
 enum I<'a> {
     ShortFlag(char),
     LongFlag(&'a str),
+    Ambiguity(&'a str),
     ShortCmd(char),
     LongCmd(&'a str),
+    Nested(&'a str),
 }
 
 // human readable
@@ -62,6 +76,8 @@ impl std::fmt::Debug for I<'_> {
             Self::LongFlag(s) => write!(f, "flag: `{}`", w_err!(Long(s))),
             Self::ShortCmd(s) => write!(f, "command alias: `{}`", w_err!(s)),
             Self::LongCmd(s) => write!(f, "command: `{}`", w_err!(s)),
+            Self::Ambiguity(s) => write!(f, "flag: {} (with one dash)", w_err!(s)),
+            Self::Nested(s) => write!(f, "command {}", w_err!(s)),
         }
     }
 }
@@ -75,22 +91,25 @@ impl std::fmt::Display for I<'_> {
             Self::LongFlag(s) => write!(f, "--{}", s),
             Self::ShortCmd(s) => f.write_char(*s),
             Self::LongCmd(s) => f.write_str(s),
+            Self::Ambiguity(s) => f.write_str(s),
+            Self::Nested(s) => f.write_str(s),
         }
     }
 }
 
-fn ins(expected: I, actual: I, variants: &mut Vec<(usize, String)>) {
+fn ins<'a>(expected: I<'a>, actual: I<'a>, variants: &mut Vec<(usize, (I<'a>, I<'a>))>) {
     variants.push((
         levenshtein(&expected.to_string(), &actual.to_string()),
-        format!(
-            "No such {:?}, did you mean `{}`?",
-            actual,
-            w_flag!(expected)
-        ),
+        (actual, expected),
     ));
 }
 
-fn inner_item(arg: &Arg, item: &Item, variants: &mut Vec<(usize, String)>) {
+fn inner_item<'a>(
+    arg: &'a Arg,
+    item: &'a Item,
+    variants: &mut Vec<(usize, (I<'a>, I<'a>))>,
+    at_top_level: bool,
+) {
     let actual: I = match arg {
         Arg::Short(s, _, _) => I::ShortFlag(*s),
         Arg::Long(s, _, _) => I::LongFlag(s.as_str()),
@@ -98,12 +117,26 @@ fn inner_item(arg: &Arg, item: &Item, variants: &mut Vec<(usize, String)>) {
             Some(s) => I::LongCmd(s),
             None => return,
         },
-        // shouldn't be reachable
-        Arg::Ambiguity(_, _) => return,
+        Arg::Ambiguity(_, os) => {
+            if let Some(s) = os.to_str() {
+                I::Ambiguity(s)
+            } else {
+                return;
+            }
+        }
     };
     match item {
         Item::Positional { .. } => {}
-        Item::Command { name, short, .. } => {
+        Item::Command {
+            name, short, meta, ..
+        } => {
+            if at_top_level {
+                let mut inner = Vec::new();
+                collect_suggestions(arg, meta, &mut inner, false);
+                if let Some((0, _)) = inner.first() {
+                    variants.push((0, (actual, I::Nested(name))));
+                }
+            }
             ins(I::LongCmd(name), actual, variants);
             if let Some(s) = short {
                 ins(I::ShortCmd(*s), actual, variants);
@@ -120,19 +153,24 @@ fn inner_item(arg: &Arg, item: &Item, variants: &mut Vec<(usize, String)>) {
     }
 }
 
-fn inner(arg: &Arg, meta: &Meta, variants: &mut Vec<(usize, String)>) {
+fn collect_suggestions<'a>(
+    arg: &'a Arg,
+    meta: &'a Meta,
+    variants: &mut Vec<(usize, (I<'a>, I<'a>))>,
+    at_top_level: bool,
+) {
     match meta {
         Meta::And(xs) | Meta::Or(xs) => {
             for x in xs {
-                inner(arg, x, variants);
+                collect_suggestions(arg, x, variants, at_top_level);
             }
         }
-        Meta::Item(item) => inner_item(arg, item, variants),
+        Meta::Item(item) => inner_item(arg, item, variants, at_top_level),
         Meta::HideUsage(meta)
         | Meta::Optional(meta)
         | Meta::Many(meta)
         | Meta::Decorated(meta, _) => {
-            inner(arg, meta, variants);
+            collect_suggestions(arg, meta, variants, at_top_level);
         }
         Meta::Skip => {}
     }
diff --git a/src/structs.rs b/src/structs.rs
index 6cbbf3a..38c530b 100644
--- a/src/structs.rs
+++ b/src/structs.rs
@@ -132,7 +132,11 @@ where
 
         #[cfg(feature = "autocomplete")]
         args.swap_comps_with(&mut comps);
-        res
+        if let Err(Error::Missing(_)) = res {
+            Err(Error::Missing(Vec::new()))
+        } else {
+            res
+        }
     }
 
     fn meta(&self) -> Meta {


Test patch:
diff --git a/tests/meta_youmean.rs b/tests/meta_youmean.rs
index 3013623..36d3741 100644
--- a/tests/meta_youmean.rs
+++ b/tests/meta_youmean.rs
@@ -43,3 +43,97 @@ fn short_cmd() {
 
     assert_eq!(r, "No such command: `c`, did you mean `b`?");
 }
+
+#[test]
+fn double_dashes_no_fallback() {
+    #[derive(Debug, Clone, Bpaf)]
+    #[bpaf(options)]
+    enum Opts {
+        Llvm,
+        Att,
+        #[bpaf(hide)]
+        Dummy,
+    }
+
+    let r = opts()
+        .run_inner(Args::from(&["-llvm"]))
+        .unwrap_err()
+        .unwrap_stderr();
+
+    // TODO: can we point out at -llvm here?
+    assert_eq!(
+        r,
+        "Expected (--llvm | --att), pass --help for usage information"
+    );
+}
+
+#[test]
+fn double_dashes_fallback() {
+    #[derive(Debug, Clone, Bpaf)]
+    #[bpaf(options, fallback(Opts::Dummy))]
+    enum Opts {
+        Llvm,
+        Att,
+        Dummy,
+    }
+
+    let r = opts()
+        .run_inner(Args::from(&["-llvm"]))
+        .unwrap_err()
+        .unwrap_stderr();
+
+    assert_eq!(
+        r,
+        "No such flag: -llvm (with one dash), did you mean `--llvm`?"
+    );
+}
+
+#[test]
+fn double_dash_with_optional_positional() {
+    #[derive(Debug, Clone, Bpaf)]
+    #[bpaf(fallback(Opts::Dummy))]
+    enum Opts {
+        Llvm,
+        Att,
+        Dummy,
+    }
+
+    let pos = positional::<String>("FILE").optional();
+    let parser = construct!(opts(), pos).to_options();
+
+    let r = parser
+        .run_inner(Args::from(&["make", "-llvm"]))
+        .unwrap_err()
+        .unwrap_stderr();
+
+    assert_eq!(
+        r,
+        "No such flag: -llvm (with one dash), did you mean `--llvm`?"
+    );
+}
+
+#[test]
+fn inside_out_command_parser() {
+    #![allow(dead_code)]
+
+    #[derive(Debug, Bpaf, Clone, PartialEq)]
+    #[bpaf(options)]
+    enum Cmd {
+        #[bpaf(command)]
+        Log {
+            #[bpaf(long)]
+            oneline: bool,
+        },
+    }
+
+    let ok = cmd().run_inner(Args::from(&["log", "--oneline"])).unwrap();
+    assert_eq!(ok, Cmd::Log { oneline: true });
+
+    // Can't parse "--oneline log" because oneline could be an argument instead of a flag
+    // so log might not be a command, but we can try to make a better suggestion.
+    let r = cmd()
+        .run_inner(Args::from(&["--oneline", "log"]))
+        .unwrap_err()
+        .unwrap_stderr();
+    assert_eq!(r, "flag: `--oneline` is not valid in this context, did you mean to pass it to command \"log\"?");
+}