Merge pull request #268 from pacak/squashing

Fancier short squashing

Changelog.md

@@ -1,5 +1,8 @@
 # Change Log
 
+## bpaf [0.9.5] - Unreleased
+- fancier squashing: parse `-abfoo` as `-a -b=foo` if b is a short argument
+
 ## bpaf [0.9.4] - 2023-08-08
 - add `help` to `ParseFlag` and `ParseArgument`
 - stop deprecating `Parser::run`

src/args.rs

@@ -180,6 +180,66 @@ impl ItemState {
     }
 }
 
+fn disambiguate_short(
+    mut os: OsString,
+    short: String,
+    short_flags: &[char],
+    short_args: &[char],
+    items: &mut Vec<Arg>,
+) -> Option<Message> {
+    // block can start with 0 or more short flags
+    // followed by zero or one short argument, possibly with a body
+
+    // keep the old length around so we can trimp items to it and push a Arg::Word
+    // if we decide to give up
+    let original = items.len();
+
+    // first flag contains the original os string for error message and anywhere purposes
+    let mut first_flag = os.clone();
+
+    for (ix, c) in short.char_indices() {
+        let tail_ix = ix + c.len_utf8();
+        let rest = &short[tail_ix..];
+
+        // shortcircuit single character short options
+        if ix == 0 && rest.is_empty() {
+            items.push(Arg::Short(c, false, std::mem::take(&mut first_flag)));
+            return None;
+        }
+        match (short_flags.contains(&c), short_args.contains(&c)) {
+            // short name that can be flag
+            (true, false) => {
+                items.push(Arg::Short(c, false, std::mem::take(&mut first_flag)));
+            }
+
+            // short name that can be argument
+            (false, true) => {
+                let adjacent_body = !rest.is_empty();
+                items.push(Arg::Short(c, adjacent_body, std::mem::take(&mut os)));
+                if adjacent_body {
+                    items.push(Arg::Word(rest.into()));
+                }
+                return None;
+            }
+
+            // neither is valid and there's more than one character. fallback to using it as a Word
+            (false, false) => {
+                items.truncate(original);
+                items.push(Arg::Word(os));
+                return None;
+            }
+
+            // ambiguity, this is bad
+            (true, true) => {
+                let msg = Message::Ambiguity(items.len(), short);
+                items.push(Arg::Word(std::mem::take(&mut os)));
+                return Some(msg);
+            }
+        }
+    }
+    None
+}
+
 pub use inner::State;
 /// Hides [`State`] internal implementation
 mod inner {
@@ -266,7 +326,7 @@ mod inner {
                 name: args.name.as_deref(),
             };
 
-            for mut os in args.items {
+            for os in args.items {
                 if pos_only {
                     items.push(Arg::PosWord(os));
                     continue;
@@ -280,55 +340,15 @@ mod inner {
                 match split_os_argument(&os) {
                     // -f and -fbar, but also -vvvvv
                     Some((ArgType::Short, short, None)) => {
-                        // this scenario can be ambiguous: -fbar can mean one of
-                        // several one char: -f -b -a -r
-                        // a single short flag -f with attached value "bar"
-                        // bpaf applies following logic:
-                        // - it can be a collection of separate flags if all the names
-                        //   are valid flag name
-                        // - it can be a short flag with a value if first character is
-                        //   a valid argument name
-                        //
-                        // if both variants are correct - we complain, if just one is correct we go
-                        // with that option, otherwise this is a strange positional.
-
-                        let mut can_be_arg = true;
-                        let mut can_be_flags = true;
-                        let mut total = 0;
-                        for (ix, c) in short.chars().enumerate() {
-                            can_be_flags &= short_flags.contains(&c);
-                            if ix == 0 {
-                                can_be_arg = short_args.contains(&c);
-                            }
-                            total = ix;
-                        }
-                        // there's no body so
-                        if total == 0 {
-                            items.push(Arg::Short(short.chars().next().unwrap(), false, os));
-                            continue;
-                        }
-
-                        match (can_be_flags, can_be_arg) {
-                            (true, true) => {
-                                *err = Some(Message::Ambiguity(items.len(), short));
-                                items.push(Arg::Word(os));
-
-                                break;
-                            }
-                            (true, false) => {
-                                for c in short.chars() {
-                                    // first gets the os, rest gets the empty
-                                    items.push(Arg::Short(c, false, std::mem::take(&mut os)));
-                                }
-                            }
-                            (false, true) => {
-                                let mut chars = short.chars();
-                                let first = chars.next().unwrap();
-                                let rest = chars.as_str();
-                                items.push(Arg::Short(first, true, os));
-                                items.push(Arg::ArgWord(rest.into()));
-                            }
-                            (false, false) => items.push(Arg::Word(os)),
+                        if let Some(msg) = super::disambiguate_short(
+                            os,
+                            short,
+                            short_flags,
+                            short_args,
+                            &mut items,
+                        ) {
+                            *err = Some(msg);
+                            break;
                         }
                     }
                     Some((ArgType::Short, short, Some(arg))) => {

tests/errors.rs

@@ -159,25 +159,33 @@ fn cannot_be_used_twice() {
 
 #[test]
 fn should_not_split_adjacent_options() {
+    let a = short('a').req_flag(0);
+    let b = pure(()).to_options().command("hello");
+    let parser = construct!(a, b).to_options();
+    let r = parser.run_inner(&["-ahello"]).unwrap_err().unwrap_stderr();
+    // can probably suggest splitting here too: `-a` `hello`
+    let expected = "no such flag: `-ahello`, did you mean `hello`?";
+    assert_eq!(r, expected);
+}
+
+#[test]
+fn should_not_split_adjacent_ambig_options() {
     let a1 = short('a').req_flag(0);
     let a2 = short('a').argument::<usize>("x");
     let a = construct!([a2, a1]);
     let c = pure(()).to_options().command("hello");
     let parser = construct!(a, c).to_options();
 
     let r = parser.run_inner(&["-a=hello"]).unwrap_err().unwrap_stderr();
-    assert_eq!(
-        r,
-        "expected `COMMAND ...`, got `hello`. Pass `--help` for usage information"
-    );
+    let expected = "expected `COMMAND ...`, got `hello`. Pass `--help` for usage information";
+    assert_eq!(r, expected);
 
     let r = parser.run_inner(&["-ahello"]).unwrap_err().unwrap_stderr();
-    assert_eq!(
-        r,
-        "expected `COMMAND ...`, got `hello`. Pass `--help` for usage information"
-    );
+    let expected = "app supports `-a` as both an option and an option-argument, try to split `-ahello` into individual options\n(-a -h ..) or use `-a=hello` syntax to disambiguate";
+    assert_eq!(r, expected);
 
-    // this one is okay
+    // this one is okay, try to parse -a as argument - it fails because "hello" is not a number, then
+    // try to parse -a as a flag - this works
     let r = parser.run_inner(&["-a", "hello"]).unwrap();
     assert_eq!(r, (0, ()));
 }

tests/params.rs

@@ -77,3 +77,28 @@ fn from_str_works_with_parse() {
     let r = parser.run_inner(&["42"]).unwrap();
     assert_eq!(r, 42);
 }
+
+#[test]
+fn squashed_short_names() {
+    let a = short('a').switch();
+    let b = short('b').argument::<String>("B");
+    let parser = construct!(a, b).to_options();
+
+    // For now instead of supporting I parse "-Obits=2048" instead as "-O" with "bits=2048" body
+    // https://github.com/pacak/bpaf/issues/121
+    //
+    // let r = parser.run_inner(&["-ab=foo"]).unwrap();
+    // assert_eq!(r.1, "foo");
+
+    // an odd one, short flag a is squashed with short argument b
+    let r = parser.run_inner(&["-ab", "foo"]).unwrap();
+    assert_eq!(r.1, "foo");
+
+    // same, but body for b is adjacent as well
+    let r = parser.run_inner(&["-abfoo"]).unwrap();
+    assert_eq!(r.1, "foo");
+
+    // normal
+    let r = parser.run_inner(&["-a", "-bfoo"]).unwrap();
+    assert_eq!(r.1, "foo");
+}