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

Better than display_order and DeriveDisplayOrder argument positioning #1807

Open
2 tasks done
I60R opened this issue Apr 11, 2020 · 25 comments
Open
2 tasks done

Better than display_order and DeriveDisplayOrder argument positioning #1807

I60R opened this issue Apr 11, 2020 · 25 comments
Labels
A-derive Area: #[derive]` macro API A-help Area: documentation, including docs.rs, readme, examples, etc... C-enhancement Category: Raise on the bar on expectations S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing

Comments

@I60R
Copy link

I60R commented Apr 11, 2020

Make sure you completed the following tasks

Describe your use case

The same as for Arg::display_order(usize) and DeriveDisplayOrder but should be easier to apply

Describe the solution you'd like

  • Works well with structopt/clap_derive flattening
  • Doesn't move both arguments to top or to the bottom like .display_order (because 999 is default for every argument).
  • DeriveDisplayOrder shouldn't override it

Alternatives, if applicable

Keep using Arg::display_order(usize) and DeriveDisplayOrder

Additional context


@pksunkara pksunkara added this to the 3.1 milestone Apr 11, 2020
@kbknapp
Copy link
Member

kbknapp commented Apr 29, 2020

This might be pretty hairy to implement, especially if we end up trying to detect loops (A says display after B, and B says display after A).

I'm leaning towards closing this feature, as it'd significantly impact the code used to generate help messages for (my subjectively) little benefit.

@I60R could you elaborate on why doesn't DeriveDisplayOrder work in your case? Or Arg::help_heading doesn't fit your use case?

@I60R
Copy link
Author

I60R commented May 6, 2020

This might be pretty hairy to implement, especially if we end up trying to detect loops (A says display after B, and B says display after A).

Okay, I understand.

@I60R could you elaborate on why doesn't DeriveDisplayOrder work in your case?

I actually use it. The problem was that it don't play well with structopt flattening, more specifically it doesn't allow to move argument into separate struct while keeping its order in --help message. This could be easily structopt problem, however it also seems that it occurs because current display_order functionality in clap is somehow limited.

Arg::display_after in clap was simply the most straightforward solution here that I was able to imagine

@kbknapp
Copy link
Member

kbknapp commented May 11, 2020

I actually use it. The problem was that it don't play well with structopt flattening, more specifically it doesn't allow to move argument into separate struct while keeping its order in --help message. This could be easily structopt problem, however it also seems that it occurs because current display_order functionality in clap is somehow limited.

Aaah ok, I understand. Yeah to me that sounds like either a feature/bug we could work on as part of clap_derive (what was structopt).

I'm open to others opinions as well, but even when flattening the order should be preserved. That sounds like a new issue to me, or perhaps we re-name this issue and update the OP with this new constraint/requirement.

@pksunkara
Copy link
Member

@I60R Could you provide us a sample code where it doesn't work? AFAIK, even flattening should make it work.

@I60R I60R changed the title Add display_after for easier arg positioning in --help message Better than display_order and DeriveDisplayOrder argument positioning May 18, 2020
@I60R
Copy link
Author

I60R commented May 18, 2020

I'm open to others opinions as well, but even when flattening the order should be preserved. That sounds like a new issue to me, or perhaps we re-name this issue and update the OP with this new constraint/requirement.

Done. I also think that I've found what argument positioning I want:

  1. With DeriveDisplayOrder we increment each arg display_order starting from 1000 but only for those that had it unspecified. Then, if multiple args sharing the same display_order they would be sorted alphabetically (default behavior).

  2. Instead or alongside with we may provide display_all method for App and ArgGroup which would be used like this:

    display_all = &[
        ("FLAGS", &[
            "first",
            "second",
            "x",
            ...
        ]),
        ("OPTIONS", &[
            "nth",
            "nth+1",
            ...
        ]),
        ...,
        ("CUSTOM SECTION", &[
            ...,
            "last-1",
            "last"
        ])
    ],

it could substitute display_order and display functions, plus add more styling options like custom section headers, separators, etc.

@kbknapp is it possible to be implemented?

@pksunkara
Copy link
Member

custom section headers

We have them. help_heading.

is it possible to be implemented?

You are not exactly giving us a sample of the code where DeriveDisplayOrder is not working in clap_derive. As I said before, flattening the struct should work with the option well.

@I60R
Copy link
Author

I60R commented May 18, 2020

We have them. help_heading.

The whole point isn't in having them, but in having them in the same place and not using them explicitly.

You are not exactly giving us a sample of the code where DeriveDisplayOrder is not working in clap_derive. As I said before, flattening the struct should work with the option well.

That's actually the problem e.g. if you have this unflattened struct:

#[derive(StructOpt)]
#[structopt(global_settings=&[DeriveDisplayOrder])]
struct Opt {
    a: bool,
    b: bool,
    c: bool,
    ...
}

and then refactors it to flattened

#[derive(StructOpt)]
#[structopt(global_settings=&[DeriveDisplayOrder])]
struct Opt {
    b: bool,
    #[structopt(flatten)]
    flattened: Flattened,
    ...
}

#[derive(StructOpt)]
struct Flattened {
    a: bool,
    c: bool,
}

you may find that the original order is messed up (b-a-c) and there's no straightforward way to fix it

@pksunkara
Copy link
Member

With DeriveDisplayOrder we increment each arg display_order starting from 1000 but only for those that had it unspecified. Then, if multiple args sharing the same display_order they would be sorted alphabetically (default behavior).

Yes, that can be implemented.

@epage epage added A-help Area: documentation, including docs.rs, readme, examples, etc... C-enhancement Category: Raise on the bar on expectations and removed C: args labels Dec 8, 2021
@epage
Copy link
Member

epage commented Dec 9, 2021

only for those that had it unspecified

This is already done. We detect defaults and don't override them. The only thing remaining is what the starting display order should be when deriving.

epage added a commit to epage/clap that referenced this issue Dec 9, 2021
Now that we use options, we can rely on that, instead of sentinels, for
detecting a default and overriding only a default.

Noticed this as part of looking at clap-rs#1807.
@epage
Copy link
Member

epage commented Dec 9, 2021

So the crux of the issue is that if the struct hierarchy doesn't match your visual hierarchy, you have to manually specify all of the display orders.

I was wondering if we could do something like App::help_heading (which has naming issues similar to #1553) but the problem is a derive user doesn't have the opportunity to set an app function outside of when they do the flatten or define the struct. We can't just do a backup-restore approach because they would need to set where to pick up the counting.

@epage epage removed this from the 3.1 milestone Dec 9, 2021
@epage epage added S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing and removed P4: nice to have labels Dec 9, 2021
@pksunkara
Copy link
Member

you have to manually specify all of the display orders.

I think the case for that would be pretty niche. I think people would just find it easy to reorder the struct.

@epage
Copy link
Member

epage commented Dec 10, 2021

If we feel its niche enough, is there anything left to do for this issue?

@pksunkara
Copy link
Member

The only thing remaining is what the starting display order should be when deriving.

I think that. Unless another issue covers that? I will leave it up to you.

epage added a commit to epage/clap that referenced this issue Feb 11, 2022
- ArgSettings are part of clap-rs#2717
- Errors are part of clap-rs#2628
- `help_heading` is part of clap-rs#1807 and clap-rs#1553
- Some misc parts are for API consistency
epage added a commit to epage/clap that referenced this issue Feb 11, 2022
- ArgSettings are part of clap-rs#2717
- Errors are part of clap-rs#2628
- `help_heading` is part of clap-rs#1807 and clap-rs#1553
- Some misc parts are for API consistency
@epage
Copy link
Member

epage commented May 9, 2022

The only remaining work is to allow setting app attributes on arguments in the derive.

Huh, so derive helper attributes can only be identifiers. Item paths seem to be used for attribute macros.

@epage
Copy link
Member

epage commented May 9, 2022

Context: Clap has context-specific builder functions. next_help_heading is one example where it will set a field on all future Args added to the Command.

    let cmd = Command::new("blorp")
        .author("Will M.")
        .about("does stuff")
        .version("1.4")
        .arg(
            arg!(-f --fake "some help")
                .required(true)
                .value_names(&["some", "val"])
                .takes_value(true)
                .use_value_delimiter(true)
                .require_value_delimiter(true)
                .value_delimiter(':'),
        )
        .next_help_heading(Some("NETWORKING"))
        .arg(
            Arg::new("no-proxy")
                .short('n')
                .long("no-proxy")
                .help("Do not use system proxy settings"),
        )
        .args(&[Arg::new("port").long("port")]);

Problem: this is not currently exposed in the derive API. Command and Arg attributes can only show up in specific places, so you can't create an attribute that affects all future fields / Args by calling a Command function via an attribute. The one exception is when #[clap(flatten)]ing

    #[derive(Debug, Clone, Parser)]
    struct CliOptions {
        #[clap(flatten)]
        options_a: OptionsA,
    }

    #[derive(Debug, Clone, Args)]
    #[clap(next_help_heading = "HEADING A")]
    struct OptionsA {
        #[clap(long)]
        should_be_in_section_a: u32,
    }

How do we support context-specific builder methods like this without forcing a #[clap(flatten)]?

Possible solutions:

Key off of the inner-attribute name and call switch which we can from that (if attr inner name is next_help_heading, then know to call that on the Command rather than the Arg). This seems brittle, only working for whats been hard coded (we might forget things, we might make assumptions for what people want to do), and can run into conflicts in function names between Args and Commands.

Have the user explicitly state what the attribute is for. I feel like this would also make what is happening clearer.

How does the user specify which kind of attribute it is? Some ideas include:

  • #[clap::command(next_help_heading = "NETWORKING")]: outer-attribute namespace Derive helper attributes can only be identifiers, not item paths
  • #[clap(command::next_help_heading = "NETWORKING")]: inner-attribute namespaces
  • #[clap_command(next_help_heading = "NETWORKING")]: psuedo-outer-attribute namespacing
  • #[clap(command(next_help_heading = "NETWORKING"))]: fully own the nested parens

Per-use outer-attributes

  • #[command(next_help_heading = "NETWORKING")]: we'd have #[command(...)], #[arg(...)], etc.

I'm leaning towards inner-attribute namespaces

@Muscraft
Copy link

psuedo-outer-attribute namespacing should not be considered unless it comes from a different crate. It could cause problems when someone is new to the codebase and wants to look up how clap_command works and there is no crate for it. #[clap(command....)] is more descriptive of where it comes from and what is going on.

As for inner-attribute namespaces vs fully own the nested parens I prefer the latter. This is mostly because I have seen other crates use it (e.g. #[serde(rename_all = "...")]). While there is no "official" style, keeping consistent with other crates is a benefit

@epage
Copy link
Member

epage commented May 11, 2022

As for inner-attribute namespaces vs fully own the nested parens I prefer the latter. This is mostly because I have seen other crates use it (e.g. #[serde(rename_all = "...")](https://serde.rs/container-attrs.html#rename_all)). While there is no "official" style, keeping consistent with other crates is a benefit

To clarify, we already do what serde does. Note the example is #[clap(next_help_heading = "SECTION")]. That is the outer/inner attribute style of serde. I'm not aware of other crates using namespacing for attributes,. of any kind. If anything, they use additional attributes but I can't remember what examples there are. Forgot to include that in the options

epage added a commit to epage/clap that referenced this issue Jun 8, 2022
We aren't enumerating arguments but values for an argument, so the name
should reflect that.

This will be important as part of clap-rs#1807 when we have more specific
attribute names.
epage added a commit to epage/clap that referenced this issue Jun 8, 2022
We aren't enumerating arguments but values for an argument, so the name
should reflect that.

This will be important as part of clap-rs#1807 when we have more specific
attribute names.
@epage epage added this to the 4.0 milestone Jun 8, 2022
epage added a commit to epage/clap that referenced this issue Jun 8, 2022
We aren't enumerating arguments but values for an argument, so the name
should reflect that.

This will be important as part of clap-rs#1807 when we have more specific
attribute names.
epage added a commit to epage/clap that referenced this issue Jun 8, 2022
We aren't enumerating arguments but values for an argument, so the name
should reflect that.

This will be important as part of clap-rs#1807 when we have more specific
attribute names.
@epage
Copy link
Member

epage commented Sep 2, 2022

#4180 renames the attributes:

/// Simple program to greet a person
#[derive(Parser, Debug)]
#[command(author, version, about, long_about = None)]
struct Args {
    /// Name of the person to greet
    #[arg(short, long)]
    name: String,

    /// Number of times to greet
    #[arg(short, long, default_value_t = 1)]
    count: u8,
}

My original plan was to support:

/// Simple program to greet a person
#[derive(Parser, Debug)]
#[command(author, version, about, long_about = None)]
struct Args {
    /// Name of the person to greet
    #[command(next_help_heading = "Foo")]
    #[arg(short, long)]
    name: String,

    /// Number of times to greet
    #[command(next_help_heading = "Bar")]
    #[arg(short, long, default_value_t = 1)]
    count: u8,
}

but alternatively we could do something like

/// Simple program to greet a person
#[derive(Parser, Debug)]
#[command(author, version, about, long_about = None)]
struct Args {
    #[command(next_help_heading = "Foo")]
    _foo: (),

    /// Name of the person to greet
    #[arg(short, long)]
    name: String,

    /// Number of times to greet
    #[command(next_help_heading = "Bar")]
    _bar: (),

    #[arg(short, long, default_value_t = 1)]
    count: u8,
}
  • Pro: More visually distinct
  • Pro: Less confusion over mixing of attributes on a field
  • Pro: Less hacky implemenation
  • Con: Requires naming the no-op field (should be neutral at compile time)

Thoughts?

@epage
Copy link
Member

epage commented Sep 5, 2022

In planning this, I forgot about #1553 which has command attributes both for variants and the container. We can''t use the attribute alone to distinguish. Maybe good enough because doing things with subcommands is so rare?

@epage
Copy link
Member

epage commented Sep 6, 2022

Another problem I ran into with this is that #[command(subcommand)] would be great to allow both container attributes and (for enums) child variant attributes.

@epage
Copy link
Member

epage commented Sep 6, 2022

To recap

Ideally, we would like to support use of at least next_display_order and next_help_heading in the middle of structs and enums (see also #1553) as there isn't a way for derive users today to apply a help heading to a group of args (or subcommands after #1553) without either doing it one by one or by flattening which can be disruptive to the code.

I had made the #[clap(...)] attributes more specific (arg, command, etc) and was looking to

  • allow #[command(...)] attributes on arg fields so they apply to the container
  • open up so subcommand, flatten, and external_subcommand could accept container attributes.

Problems

  • subcommand on a variant already accepts attributes for the variant itself because it will be a subcommand
  • There no way to force #[command(...] attribute on a variant to apply to the container

One alternative I had considered is just special casing next_help_heading and next_display_order but

  • That muddles the waters for explaining to users the different attributes
  • It still runs unto problems with #[command(subcommand)] enum variants

Out-there ideas

  • Punt and allow this to work for deriving Args but not Subcommand since most of this customization will be on Subcommand`
    • Should #[command(subcommand)] still not allow other attributes within an Args so that the attributes have a single meaning? If so, the workaround for anything applied there is to put it on the enum.
    • Lack of consistency can make learning more difficult
  • Add a new #[parent(...)] or #[super(...)] attribute
    • Not as clear that it is tied to clap, could run into conflicts
  • Add #[command(super::next_help_heading = ...)] support
  • Add a #[command(super, next_help_heading = ...)] kind
    • This would require adding no-op fields and variants. The latter would be unergonomic for a user to deal with due to exhaustiveness of match

@pksunkara and anyone else, I'd appreciate input on this.

@epage
Copy link
Member

epage commented Sep 7, 2022

I don't think any of the proposed options require breaking changes beyond what is already merged, so going to push this out of 4.0.0.

@epage epage removed this from the 4.0 milestone Sep 7, 2022
@epage epage added the A-derive Area: #[derive]` macro API label Sep 16, 2022
epage added a commit to epage/clap that referenced this issue Sep 16, 2022
Originally, I saw the ideal as the parent command being isolated from
`#[clap(flatte)]` especially after all of the doc comment
leakage issues.  We scaled that back to just `next_help_heading` because
of the issues with settling on a policy and maintenance to cover
everything.  When doing `next_display_order`, we decided it would mess
things up too much to isolate it.

With clap-rs#1807, we instead have been moving towards setting
`#[command(next_help_heading)]` anywhere, we just need to finish working
out how it should work.
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 A-help Area: documentation, including docs.rs, readme, examples, etc... C-enhancement Category: Raise on the bar on expectations S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing
Projects
None yet
Development

No branches or pull requests

5 participants