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

Don't leak help_heading when flattening #2803

Closed
2 tasks done
epage opened this issue Oct 1, 2021 · 4 comments · Fixed by #2895
Closed
2 tasks done

Don't leak help_heading when flattening #2803

epage opened this issue Oct 1, 2021 · 4 comments · Fixed by #2895
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies
Milestone

Comments

@epage
Copy link
Member

epage commented Oct 1, 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

v3.0.0-beta.4

Minimal reproducible code

use clap::Clap;

#[derive(Debug, Clone, Clap)]
#[clap(help_heading = "General")]
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)]
struct OptionsB {
    #[clap(long)]
    option_b: Option<u32>,
}

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

Steps to reproduce the bug with the above code

cargo run -- --help

Actual Behaviour

option_b is in "HEADING A"

Expected Behaviour

option_b should be in `General

Additional Context

This is split out of #2785.

Similarly, we should also support setting help_heading when specifying flatten and it should push and pop the heading.

Debug Output

No response

@epage epage added the C-bug Category: Updating dependencies label Oct 1, 2021
@epage epage added this to the 3.0 milestone Oct 1, 2021
@epage
Copy link
Member Author

epage commented Oct 1, 2021

I'm assuming we'd need to add a get_heading_help and have the derive get it and then restore it afterwards.

@pksunkara pksunkara added the A-derive Area: #[derive]` macro API label Oct 1, 2021
@kbknapp
Copy link
Member

kbknapp commented Oct 1, 2021

Oddly enough I ran into exactly this at work the other day.

I'm assuming we'd need to add a get_heading_help and have the derive get it and then restore it afterwards.

Yeah that's the best way I can think of to do this in the least obtrusive manner. An alternative would be to have #[clap(help_heading = "Foo"] struct Foo; use Arg::help_heading individually on each field when creating the Arg struct instead of setting the "global" App help heading which sets the current heading for all future args added to the App until it's changed or reset.

@epage
Copy link
Member Author

epage commented Oct 1, 2021

An alternative would be to have #[clap(help_heading = "Foo"] struct Foo; use Arg::help_heading individually on each field when creating the Arg struct instead of setting the "global" App help heading which sets the current heading for all future args added to the App until it's changed or reset.

Forgot to mention that I had considered this. I assumed people would want the setting to be inherited which this wouldn't allow. For example, clap-verbosity-flag probably wouldn't be so prescriptive as setting its own help heading, relying on inheriting it. We could extend the trait to allow passing in a heading, but I worry about that being a slippery slope for what goes in the trait interface.

Granted, now that I say this, the converse to this also needs to be implemented If someone sets the heading on a flatten, that should apply as an App heading and be reset afterwards so someone can set the heading on the clap-verbosity flags.

@pksunkara
Copy link
Member

Related to #2785

epage added a commit to epage/clap that referenced this issue Oct 14, 2021
In part, this is just fixing a papercut where someone will try to use
the API in the same way between the two but it fails and they'll have to
consult the docs / rust-analyzer.

The bigger reason is that this is more derive-friendly for dealing with clap-rs#2803
since we will be able to just ask for the current help heading
before running the app and then restore it back, rather than having to
conditionalize the revert logic.
epage added a commit to epage/clap that referenced this issue Oct 14, 2021
This will help for clap-rs#2803 so we can capture and restore the help heading
around flattened derives.
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
epage added a commit to epage/clap that referenced this issue Oct 15, 2021
When working on clap-rs#2803, I noticed a disprecancy in the augment
behavior between `Arg`s and `Subcommand`s that makes it so `Arg`s can't
be flattened with the update functionality.
epage added a commit to epage/clap that referenced this issue Oct 15, 2021
When working on clap-rs#2803, I noticed a disprecancy in the augment
behavior between `Arg`s and `Subcommand`s that makes it so `Arg`s can't
be flattened with the update functionality.
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
epage added a commit to epage/clap that referenced this issue Oct 15, 2021
When working on clap-rs#2803, I noticed a disprecancy in the augment
behavior between `Arg`s and `Subcommand`s that makes it so `Arg`s can't
be flattened with the update functionality.
epage added a commit to epage/clap that referenced this issue Oct 16, 2021
Before, when `flatten`ing, the struct you flattened in could set the
help heading for all the following arguments.  Now, we scope each
`flatten` to not do that.

I had originally intended to bake this into the initial / final methods
but that would have required some re-work to allow capturing state
between them that seemed unnecessary.

Fixes clap-rs#2803
@bors bors bot closed this as completed in 65e4bf2 Oct 16, 2021
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