Flag subcommands

[NickHackman]

Closes #1361

• Adds support for Arch linux Pacman-style subcommands
• 8 tests for FlagSubCommands
• 1 Example that shows how to write an application similar to Pacman

I tried to catch all the issues, but with how large Clap is it's totally possible I missed one here or there.

Request review from @CreepySkeleton 😄

Feedback

Here are places where I think issues/inconsistencies are most likely to occur in the changes I've made. It's possible there are more, but these are places where further discussion may be necessary.

• FlagSubCommands supports using subcommands normally and with flags

$ pacman sync --search
$ pacman -S --search
$ pacman --sync --search

Personally, I think this gives a lot of freedom to users. This is a differing/controversial thing among AUR Helpers.

• Long flags don't support inferring, initially I thought this would be nice, but after further thought it's way more likely that it'll end up causing weird behavior with long flags. Totally open to adding this back 👍
• Documentation in src/build/app/flag_subcommand
• FlagSubCommand API, I think new, new_short, new_long, and with_name are consistent with the current API and are descriptive, in most cases no one will use any of the others than new.
• Hacks inside of src/parse/parser.rs specifically in maintaining state between parsers in parse_subcommand, and Input::previous. This is all done to re-visit the current short argument list, but skip over the subcommand flag.

Any feedback is greatly appreciated 😃

[NickHackman]

After further thought, it makes sense instead of a FlagSubCommand struct to just add App::long_flag and App::short_flag, since they still allow the use of a subcommand in the normal fashion.

I also realized that I missed validation specifically having Subcommand flags conflict with Arg flags, sorry!

EDIT: converted to draft

[pksunkara]

A good start, but yes, we should add the flags options to App and not create a new FlagSubcommand.

[NickHackman]

I'm still probably missing something, but I think all that's remaining is changing usage to instead of always printing the subcommand name to print the flag in the cases of it being using.

I'm probably glossing over YAML?

Ideally, the usage would print all possible options...

23_flag_subcommands_pacman-query 
Query the package database.

USAGE:
    23_flag_subcommands_pacman query [OPTIONS]

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

OPTIONS:
    -i, --info <info>...        view package information (-ii for backup files)
    -s, --search <search>...    search locally-installed packages for matching strings

Would ideally have

USAGE:
    23_flag_subcommands_pacman query [OPTIONS]
    23_flag_subcommands_pacman -Q [OPTIONS]
    23_flag_subcommands_pacman --query [OPTIONS]

But that results in issues in the implementation, because we set the bin_name during parsing and store it as a Option<String> rather than a Vec and it feels bad to use a Vec to 9 times out of 10 store a singular String. Would love some advice on handling that 😄

I think a not bad solution would be to show the flag instead of the name when it was used.

[CreepySkeleton]

Nick, thank you for pushing it forward! I haven't been able to review it thoroughly just yet - there aren't enough hours in the day - but I'll do my best to get it over with in a couple of days.

I agree with Pavan regarding App. The main reasoning for having a separate FlagSubCommand was to allow -Q subcommands without having to allow normal, query subcommands. It's gone out of window, so no reasons for separate facilities exist anymore.

Btw, what is [``] in doc comments?

[NickHackman]

@CreepySkeleton Yeah, I ended up going this route because it was relatively easier. I'm completely open to either going back that route or towards an AppSetting that would require a flag subcommand be used rather than named.

I personally like the freedom of choice for users of this route, but it makes the subcommands less explicit. Totally open to different ideas here 😄

[CreepySkeleton]

FlagSubCommands supports using subcommands normally and with flags

Let's just go with current implementation. I don't have strong feelings on the matter, and given the myriads of opinions people can have about this, I wary if we start evaluating pros and cons here, it will spawn yet another mega thread of "I like this, I like that", which is something I would like to avoid. We can always add only_flag_subcommand(bool) method in future.

Long flags don't support inferring, initially I thought this would be nice, but after further thought it's way more likely that it'll end up causing weird behavior with long flags. Totally open to adding this back 👍

What weird behavior were you referring to? I'm failing to imagine which conflicts it would case except for "t is prefix for both trim and test" but those are already handled rather well.

Hacks inside of src/parse/parser.rs specifically in maintaining state between parsers in parse_subcommand, and Input::previous. This is all done to re-visit the current short argument list, but skip over the subcommand flag.

I'll need to think about it more in-depth. It doesn't looks good, I agree.

I'm probably glossing over YAML?

What does YAML has to do with any of this?

But that results in issues in the implementation, because we set the bin_name during parsing and store it as a Option rather than a Vec and it feels bad to use a Vec to 9 times out of 10 store a singular String. Would love some advice on handling that 😄

Don't you dare feel bad about this!

This is something people call premature optimization. You see, it only makes sense to optimize critical paths (there are rare exceptions). Optimizing something that takes about 0.1% of runtime is kinda pointless and this case is one such. We set bin_name only once per parsing - very likely per program run! - and it's only ever used in generation of help/error messages which is already quite slow, not to mention that IO would induce much more of slowdown. The strictly speaking unneeded allocation simply doesn't matter because it's a rounding error.

Clap is already heavily reliant on small temporary allocations, example, and that was an example from hot path! It's been done this way because it was easy for the programmer to write, easy for a reader to understand, and fast enough. Saving a single bin_name in a Vec may be a waste, but totally unimportant waste because it's fast enough.

If we somehow come to conclusion that these small allocations do matter, we can simply make use of https://github.com/servo/rust-smallvec, thus gaining performance boost and saving some memory without having to addle our heads over this stuff.

From ergonomics point of view, I don't see much difference between Vec<String> and Option<String> in this case.

[NickHackman]

@CreepySkeleton

What weird behavior were you referring to? I'm failing to imagine which conflicts it would case except for "t is prefix for both trim and test" but those are already handled rather well.

Yeah, you're right. I'll add that 😄

I'll need to think about it more in-depth. It doesn't looks good, I agree.

There's really no good way to re-visit the current argument, because of the recursive structure of the parser. It makes sense for it to be recursive, but it just makes edge cases like this a little ugly

What does YAML has to do with any of this?

Adding support for flag subcommands in the YAML implementation of Clap

From ergonomics point of view, I don't see much difference between Vec and Option in this case.

Sweet, I'll work on that

[pksunkara]

Wait, I still don't get why a FlagSubcommand is needed instead of a setting or function on App.

[CreepySkeleton]

Sorry, got a bit (a lot) sidetracked. I'll be much more active here within the upcoming week or so 🙏.

OK, I reviewed the parsing algorithm. I don't like the way it's done, but I see no better way around. I guess we'll have to make do with what we have 😢.

Regarding the result, I really don't like that short options can appear between short subcommands, like

let matches = App::new("test")
        .subcommand(
            App::new("some")
                .short_flag('S')
                .long_flag("some")
                .arg(Arg::from("-f, --flag 'some flag'"))
                .arg(Arg::from("-p, --print 'print something'"))
                .subcommand(
                    App::new("result")
                        .short_flag('R')
                        .long_flag("result")
                        .arg(Arg::from("-f, --flag 'some flag'"))
                        .arg(Arg::from("-p, --print 'print something'")),
                ),
        )
        .get_matches_from(vec!["myprog", "-SfpRfp"]); // should be -Sfp -Rfp instead

I just personally think this is very confusing and error prone, but that's not an out-and-out no. Do you think this kind of usage is of any use? I also see that prohibiting that would introduce more complexity to the parser which is an extra point to keeping it as is.

Overall, your works is great, especially considering that the parser's code is far from being a masterpiece. Keep it coming!

[CreepySkeleton]

Wait, I still don't get why a FlagSubcommand is needed instead of a setting or function on App.

It isn't needed anymore, we've got past it. It's now an everyday normal app all the way down.

[NickHackman]

@CreepySkeleton, @pksunkara I think this is now ready for review, I probably need to rebase quite a bit, but other than that I think the features for Pacman style flag subcommands is covered. 👍

Completely open to feedback 🙂

[pksunkara]

Just minor stuff. Please create new commits addressing these rather than overwriting the existing commits. Thanks.

[pksunkara]

Just this left and then we can do the merge.

[CreepySkeleton]

Please let me review it first. I'll get to it tomorrow

[CreepySkeleton]

I finally reviewed this 🎉 Sorry for the delay, I must admit I forgot about this.

The only serious flaw I see is that flag subcommands aren't shown in --help output (SUBCOMMANDS section). I really think they should be.

[NickHackman]

@CreepySkeleton it does, I thought it would be good to add! It follows a similar structure to flags.

Help

pacman 5.2.1
Pacman Development Team
package manager utility

USAGE:
    23_flag_subcommands_pacman <SUBCOMMAND>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

SUBCOMMANDS:
    -Q, --query, query    Query the package database.
    -S, --sync, sync      Synchronize packages.
    help                  Prints this message or the help of the given subcommand(s)

Usage

23_flag_subcommands_pacman-sync 
Synchronize packages.

USAGE:
    23_flag_subcommands_pacman {sync, --sync, -S} [FLAGS] [OPTIONS] [--] [package]...

ARGS:
    <package>...    packages

FLAGS:
    -h, --help       Prints help information
    -i, --info       view package information
    -V, --version    Prints version information

OPTIONS:
    -s, --search <search>...    search remote repositories for matching strings

Is how usage is displayed, I'm open to changing the syntax here, it's just what pacman uses so I went with it.

[NickHackman]

@CreepySkeleton

I think I would prefer to fallback to good old prose instead of ASCII art. The graph above gives me strong impression that the subcommands are all "on one level" when in fact they are subcommands of subcommands of subcommands.

Sorry, this is me poorly communicating in the threads in this PR. They are all on the same level, and therefore conflict with one another.

pacman -S # valid

pacman -QS # invalid

I really like your suggested documentation though, it's clear and concise! I'll remove the bits that I caused confusion on.

[CreepySkeleton]

Being clear and concise apparently goes toe-to-toe with being non-native English speaker :)

Thank you for the hard work Nick, and may you code work flawlessly and your debugger always find bugs before they land to production!

bors r+

[bors]

Build succeeded:

• continuous-integration/travis-ci/push
• CI
• clap-rs.clap