Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stabilise AppSettings::Multicall Tracking Issue #2861

Closed
7 tasks done
fishface60 opened this issue Oct 12, 2021 · 9 comments · Fixed by #3684
Closed
7 tasks done

Stabilise AppSettings::Multicall Tracking Issue #2861

fishface60 opened this issue Oct 12, 2021 · 9 comments · Fixed by #3684
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-tracking-issue Category: A tracking issue for an unstable feature S-blocked Status: Blocked on something else such as an RFC or other implementation work.

Comments

@fishface60
Copy link
Contributor

fishface60 commented Oct 12, 2021

@pksunkara

This comment has been minimized.

@fishface60

This comment has been minimized.

@epage
Copy link
Member

epage commented Oct 14, 2021

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 Multicall has a performance impact on start-up #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
Copy link
Member

epage commented Oct 14, 2021

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
Copy link
Contributor Author

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 epage added A-parsing Area: Parser's logic and needs it changed somehow. S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed C: settings labels Dec 8, 2021
@epage
Copy link
Member

epage commented Dec 16, 2021

@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
Copy link
Member

epage commented May 2, 2022

#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
Copy link
Member

epage commented May 2, 2022

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
Copy link
Member

epage commented May 3, 2022

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.

epage added a commit to epage/clap that referenced this issue May 3, 2022
`multicall` allows you to have one binary expose itself as multiple
programs, like busybox does.  This also works well for user clap for
parsing REPLs.

Fixes clap-rs#2861
epage added a commit to epage/clap that referenced this issue May 3, 2022
`multicall` allows you to have one binary expose itself as multiple
programs, like busybox does.  This also works well for user clap for
parsing REPLs.

Fixes clap-rs#2861
epage added a commit to epage/clap that referenced this issue May 4, 2022
`multicall` allows you to have one binary expose itself as multiple
programs, like busybox does.  This also works well for user clap for
parsing REPLs.

Fixes clap-rs#2861
epage added a commit to epage/clap that referenced this issue May 20, 2022
`multicall` allows you to have one binary expose itself as multiple
programs, like busybox does.  This also works well for user clap for
parsing REPLs.

Fixes clap-rs#2861
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-tracking-issue Category: A tracking issue for an unstable feature S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants