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

Section headings don't match contents #2785

Closed
2 tasks done
stevenengler opened this issue Sep 24, 2021 · 11 comments · Fixed by #2882
Closed
2 tasks done

Section headings don't match contents #2785

stevenengler opened this issue Sep 24, 2021 · 11 comments · Fixed by #2882
Assignees
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies
Milestone

Comments

@stevenengler
Copy link
Contributor

stevenengler commented Sep 24, 2021

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Rust Version

rustc 1.55.0 (c8dfcfe04 2021-09-06)

Clap Version

master

Minimal reproducible code

use clap::Clap;

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

    #[clap(flatten)]
    options_b: OptionsB,
}

#[derive(Debug, Clone, Clap)]
#[clap(help_heading = "HEADING A")]
struct OptionsA {
    #[clap(long)]
    should_be_in_section_a: Option<u32>,
}

#[derive(Debug, Clone, Clap)]
#[clap(help_heading = "HEADING B")]
struct OptionsB {
    #[clap(long)]
    should_be_in_section_b: Option<u32>,
}

fn main() {
    CliOptions::parse();
}

Steps to reproduce the bug with the above code

cargo run -- --help

Actual Behaviour

clap-failure-example

USAGE:
    clap-failure-example [OPTIONS]

FLAGS:
    -h, --help       Print help information
    -V, --version    Print version information

OPTIONS:
        --should-be-in-section-a <SHOULD_BE_IN_SECTION_A>

HEADING A:
        --should-be-in-section-b <SHOULD_BE_IN_SECTION_B>

Expected Behaviour

clap-failure-example

USAGE:
    clap-failure-example

FLAGS:
    -h, --help       Print help information
    -V, --version    Print version information

HEADING A:
        --should-be-in-section-a <SHOULD_BE_IN_SECTION_A>

HEADING B:
        --should-be-in-section-b <SHOULD_BE_IN_SECTION_B>

Additional Context

This was broken by #2531.

Debug Output

No response

@stevenengler stevenengler added the C-bug Category: Updating dependencies label Sep 24, 2021
@pksunkara
Copy link
Member

Thanks for digging into this more than needed. Always appreciate the extra info.

@pksunkara pksunkara added the A-derive Area: #[derive]` macro API label Sep 24, 2021
@epage
Copy link
Member

epage commented Sep 24, 2021

This was broken by #2531.

App::help_heading is required to be called before adding args but in #2531, we switched to calling app methods after args.

@epage
Copy link
Member

epage commented Sep 24, 2021

My proposal is still the same, we revert #2531 and instead move setting of help/about to into_app. Args should not be setting these while Claps should.

@pksunkara pksunkara added this to the 3.0 milestone Sep 24, 2021
@epage
Copy link
Member

epage commented Sep 24, 2021

@pksunkara I wouldn't classify this as easy until we have a decision made on what we want the behavior to be because help_heading is at tension with the doc comment.

@epage
Copy link
Member

epage commented Sep 24, 2021

Note: this can be worked around by setting the help heading on each arg. Not ideal but one way to be unblocked.

@pksunkara
Copy link
Member

I think just the first arg in each flattened struct should be enough as a workaround IIRC.

#[derive(Debug, Clone, Clap)]
struct OptionsA {
    #[clap(long)]
    #[clap(help_heading = "HEADING A")]
    should_be_in_section_a: Option<u32>,
}

@pksunkara
Copy link
Member

I will leave the decision of 3.0 milestone to you. I am not sure if what you are proposing needs breaking changes or not.

@stevenengler
Copy link
Contributor Author

stevenengler commented Sep 24, 2021

Thanks for the quick replies!

I think just the first arg in each flattened struct should be enough as a workaround IIRC.

It looks like it needs to be set on each individually:

use clap::Clap;

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

    #[clap(flatten)]
    options_b: OptionsB,
}

#[derive(Debug, Clone, Clap)]
struct OptionsA {
    #[clap(long)]
    #[clap(help_heading = Some("HEADING A"))]
    should_be_in_section_a: Option<u32>,

    #[clap(long)]
    should_also_be_in_section_a: Option<u32>,
}

#[derive(Debug, Clone, Clap)]
struct OptionsB {
    #[clap(long)]
    #[clap(help_heading = Some("HEADING B"))]
    should_be_in_section_b: Option<u32>,

    #[clap(long)]
    should_also_be_in_section_b: Option<u32>,
}

fn main() {
    CliOptions::parse();
}
clap-failure-example

USAGE:
    clap-failure-example [OPTIONS]

FLAGS:
    -h, --help       Print help information
    -V, --version    Print version information

OPTIONS:
        --should-also-be-in-section-a <SHOULD_ALSO_BE_IN_SECTION_A>
        --should-also-be-in-section-b <SHOULD_ALSO_BE_IN_SECTION_B>

HEADING A:
        --should-be-in-section-a <SHOULD_BE_IN_SECTION_A>

HEADING B:
        --should-be-in-section-b <SHOULD_BE_IN_SECTION_B>

@epage
Copy link
Member

epage commented Sep 24, 2021

I will leave the decision of 3.0 milestone to you. I am not sure if what you are proposing needs breaking changes or not.

My proposed change would not be an API breakage but a behavior breakage (expectations for what deriving Args does)

@epage
Copy link
Member

epage commented Sep 24, 2021

It looks like it needs to be set on each:

Thanks for confirming this. I was making my assumption based on doc string

Set a custom heading for this arg to be printed under

https://docs.rs/clap/3.0.0-beta.4/clap/struct.Arg.html#method.help_heading

@pksunkara
Copy link
Member

epage added a commit to epage/clap that referenced this issue Oct 5, 2021
Before clap-rs#2005, `Clap` was a special trait that derived all clap traits it
detected were relevant (including an enum getting both `ArgEnum`,
`Clap`, and `Subcommand`).  Now, we have elevated `Clap`, `Args`,
`Subcommand`, and `ArgEnum` to be user facing but the name `Clap` isn't
very descriptive.

This also helps further clarify the relationships so a crate providing
an item to be `#[clap(flatten)]` or `#[clap(subcommand)]` is more likely
to choose the needed trait to derive.

Also, my proposed fix fo clap-rs#2785 includes making `App` attributes almost
exclusively for `Clap`.  Clarifying the names/roles will help
communicate this.

For prior discussion, see clap-rs#2583
epage added a commit to epage/clap that referenced this issue Oct 7, 2021
Before clap-rs#2005, `Clap` was a special trait that derived all clap traits it
detected were relevant (including an enum getting both `ArgEnum`,
`Clap`, and `Subcommand`).  Now, we have elevated `Clap`, `Args`,
`Subcommand`, and `ArgEnum` to be user facing but the name `Clap` isn't
very descriptive.

This also helps further clarify the relationships so a crate providing
an item to be `#[clap(flatten)]` or `#[clap(subcommand)]` is more likely
to choose the needed trait to derive.

Also, my proposed fix fo clap-rs#2785 includes making `App` attributes almost
exclusively for `Clap`.  Clarifying the names/roles will help
communicate this.

For prior discussion, see clap-rs#2583
epage added a commit to epage/clap that referenced this issue Oct 10, 2021
Before clap-rs#2005, `Clap` was a special trait that derived all clap traits it
detected were relevant (including an enum getting both `ArgEnum`,
`Clap`, and `Subcommand`).  Now, we have elevated `Clap`, `Args`,
`Subcommand`, and `ArgEnum` to be user facing but the name `Clap` isn't
very descriptive.

This also helps further clarify the relationships so a crate providing
an item to be `#[clap(flatten)]` or `#[clap(subcommand)]` is more likely
to choose the needed trait to derive.

Also, my proposed fix fo clap-rs#2785 includes making `App` attributes almost
exclusively for `Clap`.  Clarifying the names/roles will help
communicate this.

For prior discussion, see clap-rs#2583
epage added a commit to epage/clap that referenced this issue Oct 10, 2021
Before clap-rs#2005, `Clap` was a special trait that derived all clap traits it
detected were relevant (including an enum getting both `ArgEnum`,
`Clap`, and `Subcommand`).  Now, we have elevated `Clap`, `Args`,
`Subcommand`, and `ArgEnum` to be user facing but the name `Clap` isn't
very descriptive.

This also helps further clarify the relationships so a crate providing
an item to be `#[clap(flatten)]` or `#[clap(subcommand)]` is more likely
to choose the needed trait to derive.

Also, my proposed fix fo clap-rs#2785 includes making `App` attributes almost
exclusively for `Clap`.  Clarifying the names/roles will help
communicate this.

For prior discussion, see clap-rs#2583
@epage epage self-assigned this Oct 14, 2021
epage added a commit to epage/clap that referenced this issue Oct 15, 2021
We normally set all app attributes at the end.  This can be changed but
will require some work to ensure
- Top-level item's doc cmment ins our over flattened
- We still support `Args` / `Subcommand` be used to initialize an `App` when
  creating a subcommand

In the mean time, this special cases `help_heading` to happen first.
We'll need this special casing anyways to address clap-rs#2803 since we'll need
to capture the old help heading before addings args and then restore it
after.  I guess we could unconditionally do that but its extra work /
boilerplate for when people have to dig into their what the derives do.

Fixes clap-rs#2785
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-bug Category: Updating dependencies
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants