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

Idea: flatten attribute could allow setting a prefix and suffix #3117

Closed
epage opened this issue Dec 9, 2021 · 20 comments
Closed

Idea: flatten attribute could allow setting a prefix and suffix #3117

epage opened this issue Dec 9, 2021 · 20 comments
Labels
A-derive Area: #[derive]` macro API S-wont-fix Status: Closed as there is no plan to fix this

Comments

@epage
Copy link
Member

epage commented Dec 9, 2021

Issue by kamalmarhubi
Sunday Jun 03, 2018 at 22:03 GMT
Originally opened as TeXitoi/structopt#114


Allow something like:

#[derive(StructOpt)]
struct Opts {
  #[structopt(flatten, prefix = "http-")]
  http_address: Address,
  #[structopt(flatten, prefix = "monitoring-", default_value = "127.0.0.1")]
  monitoring_address: Address,
}

#[derive(StructOpt)]
sturct Address {
  #[structopt(short = "p", long = "port")]
  port: u16,
  #[structopt(short = "a", long = "address")]
  address: IpAddr
}

The goal being to end up with something like

myapp --http-address 0.0.0.0 --http-port 80 --monitoring-port 9876

There's some annoying semantic stuff that might make this impossible:

  • what do you do with short flags on the flattened struct? Ignore them?
  • how do you pass default server and port in default_value? Does the type need to impl FromStr in such a way that it does the desired thing? Eg, in this case maybe default_value = "ip:port" where either part can be left out and not set a default for it. But then validation gets weird.

Also unclear if there are more use cases. It fits well here, but who knows about other places!


This came up from thinking about clap-rs/clap-port-flag#2 (review).

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by TeXitoi
Friday Oct 12, 2018 at 10:02 GMT


This is implemented except the prefix feature

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by lucab
Tuesday Mar 26, 2019 at 10:28 GMT


I hit a weird corner-case in flatten due to the lack of prefixing. The following will panic at runtime due to children structs having fields with the same name ("port"):

#[macro_use]
extern crate structopt;

#[derive(Debug, StructOpt)]
struct Opts {
    #[structopt(flatten)]
    incoming: Incoming,
    #[structopt(flatten)]
    outgoing: Outgoing,
}

#[derive(Debug, StructOpt)]
struct Incoming {
    #[structopt(long = "port-in")]
    port: u16,
}

#[derive(Debug, StructOpt)]
struct Outgoing {
    #[structopt(long = "port-out")]
    port: u16,
}

fn main() {
    use structopt::StructOpt;

    let cli = Opts::from_args();
    println!("{:#?}", cli);
}
$ cargo run -q

thread 'main' panicked at 'Non-unique argument name: port is already in use', /home/lucab/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.32.0/src/app/parser.rs:174:9

My current workaround is to disambiguate colliding fields with unique structopt-names, e.g.

#[structopt(name = "outgoing.port", long = "port-out")]

/cc @steveej FYI

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by CreepySkeleton
Tuesday Sep 17, 2019 at 09:06 GMT


I don't think the prefix thing is possible at compile time because we can't pass info between macro calls (every struct/enum is a separate macro call) but it seems we may be able generate code that dispatches this at runtime

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by qmx
Friday Sep 20, 2019 at 23:19 GMT


@CreepySkeleton wouldn't we be able to do this by optionally making flatten a method?

#[structopt(flatten(prefix = "foo", suffix = "bar"))]

one thing I couldn't figure out is how would we make it work both as a sole identifier and as a method to not break current usage (without making it too ugly 😛)

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by CreepySkeleton
Saturday Sep 21, 2019 at 08:42 GMT


@qmx It's not about precise syntax, it's about technical complexity. The main problem is: macro calls are independent, the expansion order is not defined, there's no way to pass some info from one invocation to another.

#[derive(StructOpt)] // first expansion
struct Opt {
    // this is the place where we *actually* have 
    // the info needed
    #[flatten(prefix = "foo")]
    a: Foo,
}

#[derive(StructOpt)] // second expansion
struct Foo {
    // this is the place where we need to *apply* 
    // this prefix info
    #[structopt(short = "p", long = "port")]
    port: u16,
}

We simply cannot pass this info on compile time. The only way to implement this is to generate code that dispatches it at runtime (i.e when user's program actually executes). This is how flatten implemented in the first place - but code for flatten is simple (~4 lines total). I really get shivers running down my spine every time I get to think about what it would take to implement this (ripping off the clap::App and rebuilding it at runtime... creepy).

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by CreepySkeleton
Saturday Sep 21, 2019 at 08:51 GMT


one thing I couldn't figure out is how would we make it work both as a sole identifier and as a method to not break current usage

The syntax looks fine to me, could you explain what it would break, exactly?

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by qmx
Saturday Sep 21, 2019 at 15:03 GMT


We simply cannot pass this info on compile time. The only way to implement this is to generate code that dispatches it at runtime (i.e when user's program actually executes).

Ooooh, now I get what you meant! Thanks for taking the time to explain it!

Even if the implementation isn't that straightforward, would the project accept a PR for this functionality? Checking before I sink time trying to figure this out 😃

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by CreepySkeleton
Saturday Sep 21, 2019 at 15:15 GMT


@qmx Um, I'm not the guy in charge here, you should be asking @TeXitoi. Personally, I have no objection

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by oberien
Thursday Dec 19, 2019 at 18:19 GMT


Any advancements on this issue? I'd really like to have this feature as well.

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by CreepySkeleton
Friday Dec 20, 2019 at 17:36 GMT


@oberien The real blocker here (apart from the lack of inter-expansion communication in proc-macros as mentioned above) is clap's API. To implement this we would need Arg::get_long(), Arg::get_short(), and Arg::get/set_name(&str) methods.

We need them because all we have at any flatten point is clap::Arg instance. To set a prefix/suffix we would need to figure out the base string to apply the prefix to and there's no way to get it for neither name, nor short, nor long.

You might say - "Come on, those fields are pub, just doc(hidden); go do the dirty trick and we're set up!", but... well, I don't know how @TeXitoi feels about it but I really don't think this is a good idea, even though clap v2.x branch is kind of frozen. I don't like setting a precedent like that, an act of usage of hidden API is worse than murdering a kitten.

But there is still a blink of light in the darkness: structopt is being imported directly into clap and, since it will be technically one repo, we could implement it with no problem.

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by TeXitoi
Saturday Dec 21, 2019 at 10:54 GMT


I agree, I prefer avoiding these kind of dirty hacks.

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by Veetaha
Thursday May 21, 2020 at 11:40 GMT


Murdering a kitten is certainly rather worse than software development minutiae, but you have the point.
So is this the future direction for structopt?
I see this notice:

This crate is used by clap, and not meant to be used directly by consumers.

Or is this just until the stable clap_derive api is settled?

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by TeXitoi
Thursday May 21, 2020 at 13:19 GMT


This warning explain that the derive crate should not be used directly, but the non derive crate export the functionalities.

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by CreepySkeleton
Thursday May 21, 2020 at 13:30 GMT


This crate is used by clap, and not meant to be used directly by consumers.

For the record: this quote is from clap_derive, not from structopt. The wording is awkward though, we'll change it to something more descriptive. The meaning here is that the derive is pretty useless on it's own, without clap since the traits are defined in clap.

For the sake of having a place to direct future questions to, I'm giving the most comprehensive answer I can come up with to the questions I've been asked before at once:

  • What is the future of structopt?
    structopt is going to continue working with clap 2.x, while clap_derive will be working with clap 3.x. structopt won't go anywhere, but will likely be receiving less attention from maintainers.
  • How clap_derive relates to structopt?
    The best description of clap_derive is "a new iteration of structopt". It will receive a number of new features, including this one.
  • When will clap_derive be released? Is it being worked on?
    We're working on it, albeit slowly. The release date is "when it's done", probably in a month or two, but no guarantees.

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Comment by albx79
Friday Oct 01, 2021 at 13:22 GMT


It would be a very useful feature, though. In case e.g. one is connecting to two databases, one can setup a #[derive(StructOpt)] struct DbConfig{...}, then import it twice in the main app config with different prefixes.

Will any future version (e.g. clap v3) be able to overcome the technical limitations?

@epage
Copy link
Member Author

epage commented Dec 9, 2021

Unfortunately, I do not see this technical limitation going away. We have limited communication when flattening and any of that communication will be at runtime, not compile time, ie we would have to do string formatting and leak the result.

@epage epage closed this as completed Dec 9, 2021
@epage epage added the A-derive Area: #[derive]` macro API label Dec 9, 2021
@pksunkara
Copy link
Member

@epage WDYT of an arg_prefix method on App which behaves similarily to help_heading? This is a good feature request IMO.

@epage
Copy link
Member Author

epage commented Dec 10, 2021

To allow an impl Args to be reused, we would need

  • Uniquification of name
  • Uniquification or banning of short
  • Uniquification of long
  • Preferably, uniquification of value name for at least positional arguments

The biggest challenge is with "Uniquification of name". Each port and address would need a unique name inside of ArgMatches. This can't just be done behind the scenes with a builder function because the impl Args needs to generate code to read from ArgMatcheswhich means it needs to know the unique name but it doesn't have it, the structure that flattened it does. We could break compatibility on the trait and pass this along but (1) that is a slipperly slope, there have already been several other cases where that is an 'easy" tool to reach for and doing so would frequently break the trait's API and bloat it and (2) this helps for one level of flatten but gets even more complicated for multiple levels of flatten.

@Sytten
Copy link

Sytten commented Nov 9, 2023

For reference I proposed here #4556 (comment) that we allow prefix in the Args derive instead than at the flatten point.

@cbeck88
Copy link

cbeck88 commented Nov 10, 2023

Does anyone know a good workaround for this?

Currently I'm contemplating taking a shared struct with derive(Parser) which is clap(flatten) at several places, replacing it with a macro that can instantiate it with several env prefixes, and making a trait that all these instantiations can implement which can access the parsed fields, and using trait objects where this config is needed.

If you see a simpler way I'm interested to hear about it

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 S-wont-fix Status: Closed as there is no plan to fix this
Projects
None yet
Development

No branches or pull requests

4 participants