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

Args/Subcommand app settings shouldn't impact what they are flattened into #2894

Closed
2 tasks done
epage opened this issue Oct 16, 2021 · 4 comments
Closed
2 tasks done
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-wont-fix Status: Closed as there is no plan to fix this

Comments

@epage
Copy link
Member

epage commented Oct 16, 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::{Args, Parser};

#[derive(Parser, PartialEq, Debug)]
struct Opt {
    #[clap(flatten)]
    common: Common,
}

#[derive(Args, PartialEq, Debug)]
#[clap(version = "3.0.0")]
struct Common {
    arg: i32,
}

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

Steps to reproduce the bug with the above code

cargo run -- --help

Actual Behaviour

test-clap 3.0.0

USAGE:
    test-clap <ARG>

ARGS:
    <ARG>    

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

Expected Behaviour

test-clap 

USAGE:
    test-clap <ARG>

ARGS:
    <ARG>    

OPTIONS:
    -h, --help    Print help information

Additional Context

With the vocabulary we are providing users, we are flattening an Args. This says nothing about flattening an App as well.

This has also led to a series of bugs like #2527. We've worked around this by carefully setting the precedence (TeXitoi/structopt#325, #2531 and #2819) which has lead to other bugs like #2785 which required more precedence tweaks (#2882).

By distinguishing what app settings are tightly associated with flattening (help_heading, the implicit subcommand required else help) and making everything else only apply when creating an App, we can simplify, clarify, and make less brittle these relationships.

Debug Output

No response

@epage epage added C-bug Category: Updating dependencies A-derive Area: #[derive]` macro API labels Oct 16, 2021
@epage
Copy link
Member Author

epage commented Oct 16, 2021

Note: we can't just move the app method calls to IntoApp because that will break subcommands because Args and Subcommand are also implicitly sub-parsers in subcommands and we expect them to be able to initialize the subcommand's app settings.

Options

  • Have Args and Subcommand also derive IntoApp
  • Have users explicitly derive IntoApp when they want to be used as a subcommand
  • Split the Args and Subcommand augment_app methods into augment_app and add_args.

@epage
Copy link
Member Author

epage commented Nov 2, 2021

In thinking about this, it might make sense for subcommand to pull in some appsettings. We already implicitly add one we'd need to keep and the user might want to do other related ones like UseLongFormatForHelpSubcommand, InferSubcommands,

@epage
Copy link
Member Author

epage commented Dec 14, 2021

In #2983 I made this comment

Earlier I had commented that #2894 is "This is bigger and riskier.". As I mentioned in #2894, there are some settings that are coupled to a Subcommands behavior, like UseLongFormatForHelpSubcommand, InferSubcommands, or help heading defaults, where it would make sense to set these on the Subcommand to centralize all of the subcommand policy.

Similarly, for Args, we have some settings on what is flattened, like help_heading. #1887 is one example of where we might extend that.

I'm concerned that trying to manage what can and can't be set in a Subcommand or an Args is going to take a decent amount of code, will be brittle as we forget to update this as new app settings become available, and when someone creatively comes up with a new idea, they will be blocked until a new clap release loosens the restrictions (looking at you, clap_derive inferred vs explicit error reporting that we've been slowly loosening)

So I wonder if we should reject #2894.

@epage epage added S-wont-fix Status: Closed as there is no plan to fix this and removed S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing labels Feb 8, 2022
@epage
Copy link
Member Author

epage commented Feb 8, 2022

After having had time with this in various scenarios, I feel like any solution for this will end up being worse than the problem.

@epage epage closed this as completed Feb 8, 2022
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 M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-wont-fix Status: Closed as there is no plan to fix this
Projects
None yet
Development

No branches or pull requests

1 participant