[WIP] Switch from structopt to clap v4

[lukehsiao]

As clap v3 is now out, and the structopt features are integrated into
(almost as-is), structopt is now in maintenance mode: no new feature
will be added [1]. Consequently, it seems appropriate to switch to
clap v3 to benefit from the continued development.

clap v3 is slightly larger than structopt. This is the affect on some
basic metrics:

| Metric                | Before | After |
|-----------------------|--------|-------|
| compile release (sec) | 6.25   | 8.00  |
| binary size (MB)      | 6.2    | 6.3   |

Also note that we break away from the default clap v3 behavior in one
case: exit codes. In the transition for v2 to v3, clap switched from an
exit status of 1 to an exit status of 2. For compatibility with previous
version of choose, we still return an exit code of 1, rather than 2.

Tested:
Ran cargo test:

test result: ok. 206 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s

Ran test/e2e_test.sh:

All tests passed

---

In addition, this PR cleans up some clippy lints while we're at it :). You can see these yourself if you checkout after the first commit and run cargo clippy with clippy v0.1.64.

I'd suggest reviewing patch-by-patch, I tried to organize the changes.

[theryangeary]

Hi @lukehsiao, thanks for the contribution. I will review it more closely soon.

[lukehsiao]

Clap just released v4: https://epage.github.io/blog/2022/09/clap4/.

I've explored using clap v4, which is smaller than v3 and would be nice to use. But, there is one implementation difference that makes choose arguments ambiguous. https://docs.rs/clap/latest/clap/builder/struct.Arg.html#method.num_args search for "a common mistake". This causes the clap v4 port to give errors like:

❯ cargo run -- 0:1 -i test/lorem.txt
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/choose '0:1' -i test/lorem.txt`
failed to parse choice argument: -i
error: Invalid value "-i" for '[CHOICES]...': invalid digit found in string

For more information try '--help'

where the other ordering works:

❯ cargo run -- 0:1 -i test/lorem.txt
    Finished dev [unoptimized + debuginfo] target(s) in 0.01s
     Running `target/debug/choose '0:1' -i test/lorem.txt`
failed to parse choice argument: -i
error: Invalid value "-i" for '[CHOICES]...': invalid digit found in string

For more information try '--help'

Looking into see if there is a way to resolve this. This could be a clap bug.

So, I suppose this PR should be updated either to clap v4, or, if you want a lighter-weight option, bpaf seems quite popular, but new and I'm not sure on compatibility.

[lukehsiao]

Ok, I believe this is a clap v4 bug.
https://docs.rs/clap/latest/clap/builder/struct.Arg.html#method.allow_hyphen_values

NOTE: If a positional argument has allow_hyphen_values and is followed by a known flag, it will be treated as a flag (see trailing_var_arg for consuming known flags). If an option has allow_hyphen_values and is followed by a known flag, it will be treated as the value for the option.

This behavior just doesn't seem to be true for our situation. Perhaps the combination of allow_hyphen_values = true and value_parser = parse::choice breaks this behavior.

[lukehsiao]

Tracking here: clap-rs/clap#4283

[lukehsiao]

I've also confirmed bpaf will not be able to replicate the current UI: https://github.com/pacak/bpaf/blob/master/examples/negative.rs

Specifically, it does not have something like allow_hyphen_values, so the positionals with negatives would have to come after --, similar to clap's last

[theryangeary]

Thanks for the thorough updates. It looks like if we want to convert to clap (whether v4 or v3) we need to hold off a bit until the negative number parsing we use is possible. Do I have that right?

[lukehsiao]

Thanks for the thorough updates. It looks like if we want to convert to clap (whether v4 or v3) we need to hold off a bit until the negative number parsing we use is possible. Do I have that right?

Almost. Clap v3 actually works fine (this PR minus the wip: commit). But...it does feel somewhat unnecessary to go to clap v3 when v4 is out.

My thought is just ignore this PR until clap v4 supports the usage.

Otherwise, if we want clap v4 before they fix (if they decide to fix it, it seems up in the air right now), we'd need to release a new choose version, as it would be a breaking change for the CLI.

[theryangeary]

Agreed. Let's wait and see if clapv4 can work for us, and if it ends up that it isn't going to work, we can decide how to proceed from there.

[pacak]

Specifically, it does not have something like allow_hyphen_values, so the positionals with negatives would have to come after --, similar to clap's last

bpaf doesn't have allow_hypen_values mostly because you can make one yourself using any, adjacent and anywhere:

consider this test case - it allows to parse -a -42 as well as -a=-42.

#[test]
fn fancy_negative() {
    let a = short('a').req_flag(());
    let b = any::<i64>("A");
    let ab = construct!(a, b).adjacent().anywhere().map(|x| x.1);

    let c = short('c').argument::<usize>("C").fallback(42);

    let parser = construct!(c, ab).to_options();

    let r = parser.run_inner(Args::from(&["-a", "-10"])).unwrap();
    assert_eq!(r, (42, -10));

    let r = parser
        .run_inner(Args::from(&["-a=-20", "-c", "110"]))
        .unwrap();
    assert_eq!(r, (110, -20));

    let r = parser
        .run_inner(Args::from(&["--help"]))
        .unwrap_err()
        .unwrap_stdout();

    let expected = "\
Usage: [-c C] -a <A>

Available options:
    -c <C>
    -a
    -h, --help  Prints help information
";
    assert_eq!(r, expected);
}

Generated help is a bit strange - it contains only -a without meta variable, but it can be fixed by adding a fake parser for a that would parse the same thing but in a regular way - it won't be able to parse -a -42 example, hiding the first one and composing those two as alternatives. First parser will be doing parsing, second parser will show up in help.

let ab = ab.hide();
let a = short('a').help("help message").argument();
let parser = construct!([ab, a])

Resulting parser parses negative values as expected and shows in help messages as expected.

The way bpaf achieves small API and good compile times is by giving users enough tools to compose any parser they might need instead of coming with lots of ready made blocks.

[lukehsiao]

Thanks @pacak!

Does this cover just plain positionals, e.g. -4:-2 -i ${test_dir}/lorem.txt? That is, not just supporting hyphen values, but hyphen values that do not come after a flag?

See a larger set of example args here:

choose/test/e2e_test.sh

Lines 9 to 26 in b9ab074

	diff -w <(cargo run -- 0:1 -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_0x1.txt")
	diff -w <(cargo run -- 0 3 -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_0_3.txt")
	diff -w <(cargo run -- :1 -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_x1.txt")
	diff -w <(cargo run -- 9 3 -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_9_3.txt")
	diff -w <(cargo run -- 9 -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_9.txt")
	diff -w <(cargo run -- 12 -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_12.txt")
	diff -w <(cargo run -- 4:2 -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_4x2.txt")
	diff -w <(cargo run -- -4:-2 -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_-4x-2.txt")
	diff -w <(cargo run -- 1:3 -o % -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_1x3of%.txt")
	diff -w <(cargo run -- 1 3 -o % -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_1_3of%.txt")
	diff -w <(cargo run -- 1 3 -o '' -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_1_3of.txt")
	diff -w <(cargo run -- 3:6 -c -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_0_3_c.txt")
	diff -w <(cargo run -- 2 -x -i ${test_dir}/lorem.txt 2>/dev/null) <(cat "${test_dir}/choose_2_x.txt")
	diff -w <(cargo run -- -1 -i ${test_dir}/alphabet.txt 2>/dev/null) <(cat "${test_dir}/choose_-1.txt")
	diff -w <(cargo run -- -2 -i ${test_dir}/alphabet.txt 2>/dev/null) <(cat "${test_dir}/choose_-2.txt")
	diff -w <(cargo run -- 1:-1 -i ${test_dir}/alphabet.txt 2>/dev/null) <(cat "${test_dir}/choose_1x-1.txt")
	diff -w <(cargo run -- 1:-2 -i ${test_dir}/alphabet.txt 2>/dev/null) <(cat "${test_dir}/choose_1x-2.txt")
	diff -w <(cargo run -- 1:-3 -i ${test_dir}/alphabet.txt 2>/dev/null) <(cat "${test_dir}/choose_1x-3.txt")

[pacak]

-4:x and -i here are both positional? If so - yes, "any" by itself can handle them.

…

On Sun, Nov 27, 2022, 23:01 Luke Hsiao ***@***.***> wrote: Thanks @pacak <https://github.com/pacak>! Does this cover just plain positionals, e.g. -4:-2 -i ${test_dir}/lorem.txt? That is, not just supporting hyphen values, but hyphen values that do not come after a flag? See a larger set of example args here: https://github.com/theryangeary/choose/blob/b9ab07470f2513d9422581de1219ecb8683b513c/test/e2e_test.sh#L9-L26 — Reply to this email directly, view it on GitHub <#55 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAQFI54G5G6FNOAK5SD2UDWKQVDBANCNFSM6AAAAAAQVL7W4Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[pacak]

To parse cargo run -- -4:-2 -i ${test_dir}/lorem.txt with -i being argument and -4:-2 - positional I'd do something like this:

let shape = any::<String>("SHAPE").parse(xxxx);
let file = short('i').argument::<FileBuf>("FILE");
let parser = construct!(file, shape).to_options();

Plus usual help, etc. where xxxx is a function that takes String and returns Result<Shape, E>, where Shape represents whatever shape you have internally and E is something that can be displayed as an error, probably String.

Turbofishes are optional if rustc can derive the type from consumers.

[pacak]

#57