clap-rs/clap

Stabilise `AppSettings::Multicall` Tracking Issue

fishface60 opened this issue · 9 comments

fishface60 commented

Original request: #1120

Original PR: #2817

Feature flag: unstable-multicall

Known issues:

pksunkara commented

Can you please fill up the known issues that are unresolved from the pr?

fishface60 commented

Can you please fill up the known issues that are unresolved from the pr?

Working on it.

  • 👍1
epage commented

Note: I had assumed #2817 was merged when preparing to write this. I've intentionally not posted it on #2817 after finding out because I don't think we should gate resolution of that PR on further design discussions

For #2864, #2865, and #2866, I wonder if we should make some simplifying assumptions. Currently, #2817 bakes in one multi-call policy and these are about the impact of that or for supporting a variety of other multi-call policies. Instead on taking on the burden to support all multi-call policies, what if we generalize multi-call policy in clap and put the burden on the developer?

Proposal: Multicall unconditionally treat the bin as a subcommand?

  • No subcommand search, resolving #2866
  • Busybox style multicall would be implemented by duplicating the subcommands between the top-level and a busybox subcommand
    • Programs that want prefix stripping would just give the top-level subcommands a prefix
  • hostname-style multi-call would be implemented as just subcommands, no duplicating needed

Benefits

  • This keeps the API smaller, making it easier to discover what features are available in clap for all clap users
  • It keeps the code smaller, helping keep out bloat from non-multi-call users

Downside

  • A little more boilerplate for multi-call users.
    • I assume only a little because someone can use functions (builder) or structs (derive) to reuse the subcommands

I'm assuming a trade off like this is worth it because the percentage of multicall users is probably fairly low and so the cost of taking on that extra boiler plate is low, in aggregate across users, compared to the cost for everyone else.

epage commented

I added a checklist item

Demonstrate we can generate completions

I didn't create an issue because I think it might strictly be possible but its going to be non-obvious. More work might be needed around this.

fishface60 commented

For #2864, #2865, and #2866, I wonder if we should make some simplifying assumptions. Currently, #2817 bakes in one multi-call policy and these are about the impact of that or for supporting a variety of other multi-call policies. Instead on taking on the burden to support all multi-call policies, what if we generalize multi-call policy in clap and put the burden on the developer?

Proposal: Multicall unconditionally treat the bin as a subcommand?

So… no matching against base program name, no checking whether the applet exists before _do_parse.

I think the way that differs from AppSettings::NoBinaryName is that app.name should be cleared, and preferably help output should describe it as applets rather than subcommands.

  • Busybox style multicall would be implemented by duplicating the subcommands between the top-level and a busybox subcommand

    • Programs that want prefix stripping would just give the top-level subcommands a prefix

So you mean something like this?

#[derive(Parser)]
struct ConnectCommand(SocketAddr);

#[derive(Parser)]
enum DeceaseCommand {
    GenerateKeys,
    Connect(ConnectCommand),
    Handshake,
    Listen,
}

#[derive(Parser)]
enum Applet {
    #[clap(name = concat!(env!("PROGRAM_PREFIX"), "decease"))]
    Decease(DeceaseCommand),
    #[clap(name = concat!(env!("PROGRAM_PREFIX"), "decease-generate-keys"))]
    DeceaseGenerateKeys,
    #[clap(name = concat!(env!("PROGRAM_PREFIX"), "decease-connect"))]
    DeceaseConnect(ConnectCommand),
    #[clap(name = concat!(env!("PROGRAM_PREFIX"), "decease-handshake"))]
    DeceaseHandshake,
    #[clap(name = concat!(env!("PROGRAM_PREFIX"), "decease-listen"))]
    DeceaseListen,
}
$ export PROGRAM_PREFIX=dcs-
$ cargo build
$ for name in decease decease-generate-keys decease-connect decease-handshake decease-listen; do ln target/debug/decease ~/bin/"$PROGRAM_PREFIX$name"; done

I think this would work okay for derive-style, since the subcommand names get mapped to an enum variant internally, though I think it could be a lot of work for builder-style to keep adding the prefix.

I think native support for prefixes would be nice to add later to reduce the duplication in the definition, but isn't needed.

epage commented

@fishface60 something I was considering is how should a user provide a sane fallback if the binary is not a recognized name?

We have #2862 about the error message but that isn't always what a user will want.

We could tell users to setup an "external subcommand" and then re-parse it with their default subcommand name. This might be rare enough that the performance hit is acceptable. This also gives users the opportunity to report it to the user as they wish (log a warning, custom error, etc).

epage commented

#3677 polished up a lot about multicall. My main concern at this point is the fallback on unrecognized binary. We are still just showing a regular "Unrecognized subcommand" error (#2862).

Possible directions the user might want to go

  • Report to the user what commands the binary can be named
    • Can check the error kind but it might be for a deeply nested subcommand
    • Options:
      • Provide a new ErrorKind: user can provide whatever behavior they want
      • Treat it the same as arg_required_else_help and print help with a failing exit code (DisplayHelpOnMissingArgumentOrSubcommand error kind): works well for actual binaries but doesn't for REPLs
  • Provide a fallback command:
    • Can be implemented manually with allow_external_subcommand(true)

I feel like a new error kind is probably the only reasonable way to go.

epage commented

Actually, can't allow_external_subcommand(true) be used for resolving all the cases? You can use it to detect an unrecognized subcommand was used on a specific part of the hierarchy and provide print help as needed, you get the default help, or you can provide custom fallback logic.

epage commented

I think we'll stablize as-is. If we get feedback that this needs a dedicated ErrorKind, then we can just wait until 4.0.