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 #114

Closed
kamalmarhubi opened this issue Jun 3, 2018 · 16 comments
Closed
Labels
enhancement We would love to have this feature! Feel free to supply a PR need-design The concrete desing is uncertain, please get involved in the discussion before sending a PR

Comments

@kamalmarhubi
Copy link

kamalmarhubi commented Jun 3, 2018

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).

@TeXitoi TeXitoi added enhancement We would love to have this feature! Feel free to supply a PR help wanted Your help is needed! Feel free to participate in the discussion or send a PR question labels Jun 4, 2018
@TeXitoi
Copy link
Owner

TeXitoi commented Oct 12, 2018

This is implemented except the prefix feature

@TeXitoi TeXitoi added need-design The concrete desing is uncertain, please get involved in the discussion before sending a PR and removed question help wanted Your help is needed! Feel free to participate in the discussion or send a PR labels Oct 12, 2018
@lucab
Copy link

lucab commented Mar 26, 2019

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

@CreepySkeleton
Copy link
Collaborator

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

@qmx
Copy link

qmx commented Sep 20, 2019

@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 😛)

@CreepySkeleton
Copy link
Collaborator

@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).

@CreepySkeleton
Copy link
Collaborator

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?

@qmx
Copy link

qmx commented Sep 21, 2019

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 😃

@CreepySkeleton
Copy link
Collaborator

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

@oberien
Copy link

oberien commented Dec 19, 2019

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

@CreepySkeleton
Copy link
Collaborator

@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.

@TeXitoi
Copy link
Owner

TeXitoi commented Dec 21, 2019

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

@Veetaha
Copy link

Veetaha commented May 21, 2020

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?

@TeXitoi
Copy link
Owner

TeXitoi commented May 21, 2020

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

@CreepySkeleton
Copy link
Collaborator

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.

@albx79
Copy link

albx79 commented Oct 1, 2021

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?

@TeXitoi
Copy link
Owner

TeXitoi commented Jan 18, 2022

This is an enhancement, and structopt is now feature frozen.

@TeXitoi TeXitoi closed this as completed Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement We would love to have this feature! Feel free to supply a PR need-design The concrete desing is uncertain, please get involved in the discussion before sending a PR
Projects
None yet
Development

No branches or pull requests

8 participants