Skip to content
This repository has been archived by the owner on Jan 1, 2022. It is now read-only.

Stabilise AppSettings::Multicall Tracking Issue #215

Open
3 of 5 tasks
epage opened this issue Dec 6, 2021 · 5 comments
Open
3 of 5 tasks

Stabilise AppSettings::Multicall Tracking Issue #215

epage opened this issue Dec 6, 2021 · 5 comments

Comments

@epage
Copy link
Owner

epage commented Dec 6, 2021

@epage
Copy link
Owner Author

epage commented Dec 6, 2021

Comment by pksunkara
Tuesday Oct 12, 2021 at 19:54 GMT


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

@epage
Copy link
Owner Author

epage commented Dec 6, 2021

Comment by fishface60
Tuesday Oct 12, 2021 at 19:56 GMT


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

Working on it.

@epage
Copy link
Owner Author

epage commented Dec 6, 2021

Comment by epage
Thursday Oct 14, 2021 at 12:41 GMT


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

epage commented Dec 6, 2021

Comment by epage
Thursday Oct 14, 2021 at 14:03 GMT


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.

@epage
Copy link
Owner Author

epage commented Dec 6, 2021

Comment by fishface60
Saturday Oct 16, 2021 at 22:08 GMT


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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant