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

derive feature: an attribute to add a prefix to all arg names in a struct, for use with flatten #3513

Open
2 tasks done
wfraser opened this issue Feb 26, 2022 · 9 comments
Open
2 tasks done
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing

Comments

@wfraser
Copy link

wfraser commented Feb 26, 2022

Please complete the following tasks

Clap Version

3.1.2

Describe your use case

I work on a large Rust project that has many different modules which have command-line options to add to the program. For readability and to reduce collisions, we prefix the name of the flags with the name of the module it belongs to. For example, we have a module log, with a struct with several fields, and each of them has a corresponding flag like --log.whatever. The main program then combines all these together into one big options struct.

We've been using docopt, which lets you omit the prefix from the struct field name at least, but you still have to type it manually in the help text for each flag, which doesn't scale well.

Clap lets me use #[clap(flatten)] to separate these options into their own struct like I've been doing with docopt, but I'd have to add the prefix to all the field names, which then leaks into all the code that accesses it (yuck). It also doesn't let me preserve the --prefix.name argument style we've already established.

What I'd like is some way to add a prefix to all the arguments used with a particular struct.

Describe the solution you'd like

A similar feature was discussed in #3117, which proposed a prefix attribute to be placed alongside flatten, at the point of the sub-struct's inclusion into the main one. This was ultimately dismissed as being ugly and/or difficult to implement given how the derive code works.

This proposal is to attach the attribute to the sub-struct itself, which is much more easily implemented, and, at least for my use case, works just as well.

I have an implementation ready (https://github.com/wfraser/clap/tree/flatten-prefix) which makes it another type of casing style, using the rename_all attribute. The precise syntax is #[clap(rename_all = "prefix:foo")]. It can be combined with another prefix style as well: #[clap(rename_all = "prefix:foo,snake")].

Example:

#[derive(Parser, Debug, PartialEq)]
struct Main {
    #[clap(long)]
    some_string: String,

    #[clap(flatten)]
    foo_args: Foo,

    #[clap(flatten)]
    bar_args: Bar,
}

#[derive(Parser, Debug, PartialEq)]
#[clap(rename_all = "prefix:foo", next_help_heading = "Foo options")]
struct Foo {
    #[clap(long)]
    some_param: String,
}

#[derive(Parser, Debug, PartialEq)]
#[clap(rename_all = "prefix:bar,pascal", next_help_heading = "Bar options")]
struct Bar {
    #[clap(long)]
    another_param: String,
}

help text:

clap-nesting

USAGE:
    clap-nesting [OPTIONS]

OPTIONS:
    -h, --help                         Print help information
        --some-string <SOME_STRING>

Foo options:
        --foo.some-param <SOME_PARAM>

Bar options:
        --bar.AnotherParam <ANOTHER_PARAM>

If this approach looks acceptable, I can submit my branch as a PR.

Alternatives, if applicable

No response

Additional Context

No response

@wfraser wfraser added the C-enhancement Category: Raise on the bar on expectations label Feb 26, 2022
@pksunkara
Copy link
Member

I was pretty sure we had an issue about this somewhere but I couldn't find it now.

This relates to #3221

@wfraser wfraser changed the title derive feature: an attribute to add a prefix to all arg names, for use with flatten derive feature: an attribute to add a prefix to all arg names in a struct, for use with flatten Feb 26, 2022
@epage epage added A-derive Area: #[derive]` macro API S-duplicate Status: Closed as Duplicate labels Feb 26, 2022
@epage
Copy link
Member

epage commented Feb 26, 2022

This sounds like #3117 which we had previously rejected. I'm going to go ahead and close this as well but if I'm mistaken, feel free to speak up!

@epage epage closed this as completed Feb 26, 2022
@wfraser
Copy link
Author

wfraser commented Feb 26, 2022

@epage: this is intended to be different from #3117, which was closed because implementing it is problematic.

Instead of #3117 which proposes to let the user of a struct determine the prefix of all its args, I want to let the struct itself determine its own prefix (the attribute is on the struct, not where it is flattened into). This greatly simplifies the implementation, because now two different Args impls don't need to cooperate.

@pksunkara sort of similar, but this doesn't have to do with env vars, and I propose implementing it in a pretty different way. In particular, this is only a clap_derive feature, and doesn't change Args at all.

Basically, I think this is substantially simpler than both of those proposals. Maybe that limits its utility too much, I don't know, but it would solve a problem for me that I don't see being solved any other way.

@epage epage reopened this Feb 27, 2022
@epage
Copy link
Member

epage commented Feb 27, 2022

Since the use case is slightly different, I'm re-opening for now.

While this more limited-scope proposal doesn't run into most of the issues in #3117, it does run into the issue of not being able to work with flatten. We could make it a compile error to set a prefix/namespace for IDs and flatten. I am concerned about having random holes within the functionality where things don't work. Documenting the derive is already tricky and documentation is only a half solution to any problem. This isn't a "no" but I am interested in hearing more thoughts on this and and to give this time to simmer to see how things might look over time.

Some quick thoughts on details of the proposal

@epage epage added S-waiting-on-decision Status: Waiting on a go/no-go before implementing and removed S-duplicate Status: Closed as Duplicate labels Feb 27, 2022
@wfraser
Copy link
Author

wfraser commented Feb 28, 2022

Yeah those are good points.

I agree, a separate attribute would probably be better. I just piggy-backed on rename_all because it made implementation so easy; a separate attribute is probably not that much more difficult though. I'll try and write up a proof-of-concept that way.

Not working with flatten is a little unfortunate, but it seems like that's a pretty inherent difficulty due to how the code generator works. From reading previous discussions, and now the code, I don't really see how that could be made to work. That said, I think attaching the prefix to the struct itself solves most use cases. The only one I don't thing it works for is people who want to re-use an args struct multiple times, with a different prefix each time.

@wfraser
Copy link
Author

wfraser commented Feb 28, 2022

Here's a proof-of-concept of a new prefix attr that can be applied at the struct level: wfraser@prefix-attr

example:

#[derive(Parser, Debug)]
struct Main {
    #[clap(short)]
    s: Option<String>,

    #[clap(long, default_value = "spaghetti")]
    some_string: String,

    #[clap(long)]
    same_name: Option<String>,

    #[clap(flatten)]
    foo_args: Foo,

    #[clap(flatten)]
    bar_args: Bar,
}

#[derive(Parser, Debug, Default)]
#[clap(prefix = "foo", next_help_heading = "Foo options")]
struct Foo {
    #[clap(long)]
    some_param: Option<String>,

    #[clap(long)]
    same_name: Option<String>, // without prefix, would conflict with the one in Main
}

#[derive(Parser, Debug, Default)]
#[clap(prefix = "bar", next_help_heading = "Bar options")]
struct Bar {
    #[clap(long, env)]
    another_param: Option<String>,
}
clap-prefixes

USAGE:
    clap-prefixes [OPTIONS]

OPTIONS:
    -h, --help                         Print help information
    -s <S>
        --same-name <SAME_NAME>
        --some-string <SOME_STRING>    [default: spaghetti]

Foo options:
        --foo.same-name <FOO_SAME_NAME>
        --foo.some-param <FOO_SOME_PARAM>

Bar options:
        --bar.another-param <BAR_ANOTHER_PARAM>    [env: BAR_ANOTHER_PARAM=]

@epage
Copy link
Member

epage commented Feb 28, 2022

The only one I don't thing it works for is people who want to re-use an args struct multiple times, with a different prefix each time.

The problem is this seems to be the majority of the cases. This is the first time someone has brought up this combination

I think this gives me a way to express a thought I was having that I couldn't put into words before. Maintainership requires managing expectations. I worry that implementing this, though it helps some people, will make it harder for us to manage expectations for reusable structs and adapting this combination of features to people's needs.

Yeah those are good points.

Some other things I seemed to have missed in the original

  • . separators are not idiomatic for option names and not something we'd want to be a default, instead - should be a default. I'd be concerned about making this yet-another-configurable behavior.
  • We are coupling namespacing in ArgMatches with namespacing of long option names (but not shorts), and namespacing of env variables.
    • I'd imagine users will want one of these without some of the others
    • We'd need to error on shorts being set

Overall, this is seeming too narrow of a use case to justify the extra logic and documentation when a user can implement all of this themselves.

Overall, I'm still leaning towards rejecting this.

@Pure-Peace
Copy link

Pure-Peace commented Dec 9, 2022

Looking forward to this feature.

Currently I use a declarative macro to create prefixed configuration structs.

#[macro_export]
macro_rules! define_db_config {
    ($vis: vis $struct_name: ident, $prefix: ident) => {
        paste::paste! {
            /// Database configurations
            #[derive(Parser, Debug)]
            $vis struct $struct_name {
                /// Database connection URL.
                #[arg(long)]
                pub [<$prefix _db_url>]: String,

                /// Set the maximum number of connections of the pool.
                #[arg(long)]
                pub [<$prefix _db_max_connections>]: Option<u32>,

                /// Set the minimum number of connections of the pool.
                #[arg(long)]
                pub [<$prefix _db_min_connections>]: Option<u32>,

                /// Set the timeout duration when acquiring a connection.
                #[arg(long)]
                pub [<$prefix _db_connect_timeout>]: Option<u64>,

                /// Set the maximum amount of time to spend waiting for acquiring a connection.
                #[arg(long)]
                pub [<$prefix _db_acquire_timeout>]: Option<u64>,

                /// Set the idle duration before closing a connection.
                #[arg(long)]
                pub [<$prefix _db_idle_timeout>]: Option<u64>,

                /// Set the maximum lifetime of individual connections.
                #[arg(long)]
                pub [<$prefix _db_max_lifetime>]: Option<u64>
            }
        }
    };
}

define_db_config!(pub DatabaseA, a);
define_db_config!(pub DatabaseB, b);

#[derive(Parser, Debug)]
pub struct DbServiceConfig {
    #[command(flatten)]
    pub db_a: DatabaseA,

    #[command(flatten)]
    pub db_b: DatabaseB,
}

@wfraser
Copy link
Author

wfraser commented May 10, 2024

It seems unlikely that this feature will ever get accepted, and I got tired of maintaining a large patch to the clap codebase, so I implemented this functionality as a proc macro that can be used alongside clap instead: https://github.com/wfraser/clap_wrapper

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-enhancement Category: Raise on the bar on expectations S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Projects
None yet
Development

No branches or pull requests

4 participants