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

clap_derive users can't set both a default help_heading and an arg-specific help_heading #2873

Closed
2 tasks done
epage opened this issue Oct 14, 2021 · 0 comments · Fixed by #2878
Closed
2 tasks done
Assignees
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies
Milestone

Comments

@epage
Copy link
Member

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

Note: this assumes #2785 is fixed

use clap::{IntoApp, Parser};

#[test]
fn arg_help_heading_applied() {
    #[derive(Debug, Clone, Parser)]
    #[clap(help_heading = "DEFAULT")]
    struct CliOptions {
        #[clap(long)]
        #[clap(help_heading = Some("HEADING A"))]
        should_be_in_section_a: Option<u32>,

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

    let app = CliOptions::into_app();

    let should_be_in_section_a = app
        .get_arguments()
        .find(|a| a.get_name() == "should-be-in-section-a")
        .unwrap();
    assert_eq!(should_be_in_section_a.get_help_heading(), Some("HEADING A"));

    let should_be_in_default_section = app
        .get_arguments()
        .find(|a| a.get_name() == "DEFAULT")
        .unwrap();
    assert_eq!(should_be_in_default_section.get_help_heading(), Some("DEFAULT"));
}

Steps to reproduce the bug with the above code

Run the test

Actual Behaviour

should_be_in_section_a is in section DEFAULT

Expected Behaviour

should_be_in_section_a is in section HEADING A

Additional Context

PR #1211 originally added help_heading with the current priority
(App::help_heading wins).

In #1958, the PR author had proposed to change this

Note that I made the existing arg's heading a priority over the one in App::help_heading

This was reverted on reviewer request because

Thanks for the priority explanation. Yes, please make the app's help
heading a priority. I can see how it would be useful when people might
want to reuse args.

Re-using args is an important use case but this makes life difficult
for anyone using help_heading with clap_derive because the user
can only call App::help_heading once per struct. Derive users can't get
per-field granularity with App::help_heading like the builder API.

As a bonus, I think this will be the least surprising to users. Users
generally expect the more generic specification (App) to be overridden by the
more specific specification (Arg). Honestly, I had thought this Issue's Expected Behavior is
how help_heading worked until I dug into the code with #2872.

In the argument re-use cases, a workaround is for the reusable arguments
to not set their help_heading and require the caller to set it as
needed

Note: I have a fix ready, just blocked on #2872.

Debug Output

No response

@epage epage added C-bug Category: Updating dependencies A-derive Area: #[derive]` macro API labels Oct 14, 2021
@epage epage added this to the 3.0 milestone Oct 14, 2021
@epage epage self-assigned this Oct 14, 2021
epage added a commit to epage/clap that referenced this issue Oct 14, 2021
PR clap-rs#1211 originally added `help_heading` with the current priority
(`App::help_heading` wins).

In clap-rs#1958, the author had proposed to change this

> Note that I made the existing arg's heading a priority over the one in App::help_heading

This was reverted on reviewer request because

> Thanks for the priority explanation. Yes, please make the app's help
> heading a priority. I can see how it would be useful when people might
> want to reuse args.

Re-using args is an important use case but this makes life difficult
for anyone using `help_heading` with `clap_derive` because the user
can only call `App::help_heading` once per struct.  Derive users can't get
per-field granularity with `App::help_heading` like the builder API.

As a bonus, I think this will be the least surpising to users.  Users
generally expect the more generic specification (App) to be overridden by the
more specific specification (Arg).  Honestly, I had thought this PR is
how `help_heading` worked  until I dug into the code with clap-rs#2872.

In the argument re-use cases, a workaround is for the reusable arguments
to **not** set their help_heading and require the caller to set it as
needed.

Fixes clap-rs#2873
@bors bors bot closed this as completed in 7f05c15 Oct 15, 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.

1 participant