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

Set default subcommand via Derive API #3857

Closed
2 tasks done
dyc3 opened this issue Jun 21, 2022 · 9 comments
Closed
2 tasks done

Set default subcommand via Derive API #3857

dyc3 opened this issue Jun 21, 2022 · 9 comments
Labels
A-builder Area: Builder API C-enhancement Category: Raise on the bar on expectations S-wont-fix Status: Closed as there is no plan to fix this

Comments

@dyc3
Copy link

dyc3 commented Jun 21, 2022

Please complete the following tasks

Clap Version

3.1.18

Describe your use case

I maintain steamguard-cli and I recently upgraded clap to v3, and switched over to the derive API. A feature of the program is that whenever you don't supply a subcommand, it generates a code.

My current solution is to have the subcommands be optional, like so:

https://github.com/dyc3/steamguard-cli/blob/2678961a4c938c153eac0b80b952999903d41861/src/cli.rs#L35-L49

This is OK, but I wanted to add an --offline flag that would only be valid when generating a code. This means I need a new Code subcommand, but adding this in it's current form would break my backwards compatibility.

Describe the solution you'd like

It's already possible to have a default subcommand, as shown in this example:

clap/examples/git.rs

Lines 32 to 39 in 731d18f

.subcommand(
Command::new("stash")
.args_conflicts_with_subcommands(true)
.args(push_args())
.subcommand(Command::new("push").args(push_args()))
.subcommand(Command::new("pop").arg(arg!([STASH])))
.subcommand(Command::new("apply").arg(arg!([STASH]))),
)
All that needs to be done is expose this via the derive api.

#[clap(subcommand, default="Code")]
sub: Subcommands

Alternatives, if applicable

Option<Subcommands> makes it so that subcommands are optional altogether, but that means any arguments you need in the None case must become global arguments, which can get confusing for end users.

Additional Context

No response

@dyc3 dyc3 added the C-enhancement Category: Raise on the bar on expectations label Jun 21, 2022
@epage
Copy link
Member

epage commented Jun 21, 2022

This is OK, but I wanted to add an --offline flag that would only be valid when generating a code. This means I need a new Code subcommand, but adding this in it's current form would break my backwards compatibility.

Why would that break backwards compatibility? Could you clarify how this is related to the default subcommands?

Option makes it so that subcommands are optional altogether, but that means any arguments you need in the None case must become global arguments, which can get confusing for end users.

How come you say they would need to be global = true? The stash example you referenced doesn't do that.

What isn't quite clear is what the concerns are for users implementing this with the existing primitives we offer besides the extra boiler plate.

All that needs to be done is expose this via the derive api.

Ideally we add new major features like this at the Builder API level so everyone can take advantage of it. As we dig in, there might be some technical details that force us into it (like if we need any debug_asserts).

The challenge we run into is we are trying to focus on providing users building blocks rather than baking in everything to help with

  • Keeping binary sizes down
  • Keeping compile times down
  • Minimizing the API surface so what we do have is more discoverable

If there are challenges with doing this with the building blocks or if this becomes important enough across our user base, we can re-evaluate native support for it.

@epage epage added S-waiting-on-decision Status: Waiting on a go/no-go before implementing A-builder Area: Builder API labels Jun 21, 2022
@dyc3
Copy link
Author

dyc3 commented Jun 21, 2022

Why would that break backwards compatibility? Could you clarify how this is related to the default subcommands?
How come you say they would need to be global = true?

Ah, I didn't realize global had a specific meaning. Let me try again.

Here's my understanding:

Suppose we have my current implementation:

#[derive(Parser)]
struct Args {
    sub: Option<Commands>
}

#[derive(Parser)]
enum Commands {
    Setup(ArgsSetup),
}

#[derive(Parser)]
struct ArgsSetup;

When sub == None, it will generate a code. The match looks something like

match args.sub {
    Some(Commands::Setup(subargs)) => run_setup(subargs),
    None => generate_code(args),
}

I want to add an --offline flag that is only valid when generating a code, which would have to look something like:

#[derive(Parser)]
struct Args {
    #[clap(long)]
    offline: bool

    sub: Option<Commands>
}

#[derive(Parser)]
enum Commands {
    Setup(ArgsSetup),
}

#[derive(Parser)]
struct ArgsSetup;

Correct me if I'm wrong, but this would make --offline a valid argument regardless of the subcommand (as in it gets parsed and doesn't cause errors, even though it's ultimately unused).

Ideally, I would like it to look something like this:

#[derive(Parser)]
struct Args {
    #[clap(subcommand, default = "Code")]
    sub: Commands
}

#[derive(Parser)]
enum Commands {
    Setup(ArgsSetup),
    Code(ArgsCode),
}

#[derive(Parser)]
struct ArgsSetup;

#[derive(Parser)]
struct ArgsCode {
    #[clap(long)]
    offline: bool
}

This way, --offline would only be valid in the context of the Code subcommand. steamguard-cli code --offline and steamguard-cli --offline would have the same effect. args_conflicts_with_subcommands was not listed in the derive API documentation, so I didn't even know it was accessible.

Regardless, the git-derive example you linked feels really unintuitive to me. I have no idea how to apply it to my situation.

@epage
Copy link
Member

epage commented Jun 21, 2022

args_conflicts_with_subcommands was not listed in the derive API documentation, so I didn't even know it was accessible.

The derive documentation delegates to the builder documentation with this statement

Raw attributes: Any Command method can also be used as an attribute, see Terminology for syntax.

  • e.g. #[clap(arg_required_else_help(true))] would translate to cmd.arg_required_else_help(true)

Regardless, the git-derive example you linked feels really unintuitive to me. I have no idea how to apply it to my situation.

The key parts

  • Subcommands under stash are optional (command: Option<StashCommands>,), allowing someone to not provide one
  • We reuse arguments between git stash and git stash push via #[clap(flatten)] push: StashPush,
  • We prevent using git stash flags with any of the git stash subcommands with #[clap(args_conflicts_with_subcommands = true)]
  • We simplify the match code by treating the git stash args as if they were passed to git stash push via let stash_cmd = stash.command.unwrap_or(StashCommands::Push(stash.push));

It sounds like doing similar what give you similar results

#[derive(Parser)]
#[clap(args_conflicts_with_subcommands = true)]
struct Args {
    #[clap(subcommand)]
    sub: Option<Commands>,

    #[clap(flatten)]
    code: ArgsCode,
}

#[derive(Parser)]
enum Commands {
    Setup(ArgsSetup),
    Code(ArgsCode),
}

#[derive(Parser)]
struct ArgsSetup;

#[derive(Parser)]
struct ArgsCode {
    #[clap(long)]
    offline: bool
}

@dyc3
Copy link
Author

dyc3 commented Jun 21, 2022

That's a really good breakdown, thank you! I'm able to get the behavior I want. I still think having derive syntax for a default subcommand would be convenient and easier to work with. Purely as a nice to have though, not missing functionality.

@gregdhill
Copy link

@epage what if some of the commands in ArgsCode are required?

For example if we try Args::parse() here we get MissingRequiredArgument.

#[derive(Parser)]
struct Args {
    #[clap(subcommand)]
    sub: Option<Commands>,

    #[clap(flatten)]
    code: ArgsCode,
}

#[derive(Parser)]
enum Commands {
    Setup(ArgsSetup),
    Code(ArgsCode),
}

#[derive(Parser)]
struct ArgsSetup;

#[derive(Parser)]
struct ArgsCode {
    #[clap(long)]
    name: String,
}

@epage
Copy link
Member

epage commented Sep 7, 2022

@gregdhill Your example would look like

#[derive(Parser)]
#[clap(args_conflicts_with_subcommands = true)]
struct Args {
    #[clap(subcommand)]
    sub: Option<Commands>,

    #[clap(flatten)]
    code: ArgsCode,
}

#[derive(Parser)]
enum Commands {
    Setup(ArgsSetup),
    Code(ArgsCode),
}

#[derive(Parser)]
struct ArgsSetup;

#[derive(Parser)]
struct ArgsCode {
    #[clap(long, required = true)]
    name: Option<String>,
}
  • args_conflicts_with_subcommands will override required = true
  • name needs to be Option<T> so we have something to put in its place when it isn't required
  • Because name is Option<T>, required = true isn't being set for us and we need to do it ourselves.

@epage
Copy link
Member

epage commented Sep 7, 2022

btw another discussion on default subcommands: #4134

@epage
Copy link
Member

epage commented Sep 7, 2022

I'm closing this because

  • We have shown that this is possible today with existing primitives
  • This seems uncommon enough that it doesn't warrant built-in behavior

If someone wants to continue the discussion to see if a solution aligns well enough with clap that we can merge it anyways, some points to consider

  • The derive API is built on top of the builder API and a design will need to include how it maps to the builder API
  • What is more likely to get supported is if you can find general primitives to make default subcommands easier but also help in other cases (e.g. instead of us merging 3-4 different multicall options, we simplified it down to just one style of multicall that all of the others can be built off of).

@epage epage closed this as completed Sep 7, 2022
@epage
Copy link
Member

epage commented Sep 8, 2022

@epage epage added S-wont-fix Status: Closed as there is no plan to fix this and removed S-waiting-on-decision Status: Waiting on a go/no-go before implementing labels Nov 2, 2022
@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-builder Area: Builder API C-enhancement Category: Raise on the bar on expectations S-wont-fix Status: Closed as there is no plan to fix this
Projects
None yet
Development

No branches or pull requests

3 participants