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.

Sign up for GitHub

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

Jump to bottom

Please default to DisableVersionForSubcommands #2812

Closed
2 tasks done
joshtriplett opened this issue Oct 4, 2021 · 5 comments · Fixed by #2831
Closed
2 tasks done

Please default to DisableVersionForSubcommands #2812

joshtriplett opened this issue Oct 4, 2021 · 5 comments · Fixed by #2831

Comments

@joshtriplett
Copy link
Contributor

Please complete the following tasks

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

Clap Version

3.0.0-beta.4

Describe your use case

I have various subcommands, each of which has various options, and in the help for those subcommands, the display space and cognitive space of the user is precious. I don't want to show the --version option on subcommands, because it takes up one more line and one more bit of "complexity at a glance".

Describe the solution you'd like

I'd propose that either DisableVersionForSubcommands should become the default, or a new HideVersionForSubcommands should become the default.

Alternatives, if applicable

I think it'd be fine if --version worked on subcommands; I just don't want it to show up in the help output. So, a HideVersionForSubcommands option would be fine, and would avoid any potential backwards compatibility issues.

Additional Context

No response
@epage
Copy link
Member

epage commented Oct 5, 2021

I'll be upfront, I'm unsure how we should go about a decision like this.

Something that might help is if we have an idea what precedent there is for current behavior and any of the proposed behaviors like we did for #2808

@epage
Copy link
Member

epage commented Oct 5, 2021

Forgot to mention, this is being carried over from #2627 (comment)

Two concerns I raised there

  • As a user, its handy to be able to add a flag to my last command and it just work, rather than having to edit the entire command or type it over
  • cargo subcmd maps to cargo-subcmd subcmd. If we make DisableVersionForSubcommands the default, then cargo subcmd --version will not work by default.
    • I wonder how often this pattern for external subcommands exists. I know git doesn't do it
    • However, cargo is likely enough for people to plug into that I worry about people releasing without knowing they are missing --version and then them having to discover how to get it back

@kbknapp
Copy link
Member

kbknapp commented Oct 6, 2021

Agreed that subcommands are rarely individually versioned. I'd be in favor of making the global version the default.

As for wanting to hide or disable --version from all subcommands I think that should be opt-in via DisableVersionForSubcommands.

Actually...now that I think about it. Why do we build/show a --version when no version exists? I.e. if the user hasn't set a version, we shouldn't present that as a flag. Granted, if we did this, my first statement about making global version the default would then still require @joshtriplett or similar use cases to do something like DisableVersionForSubcommands.

@kbknapp
Copy link
Member

kbknapp commented Oct 6, 2021

To be clear, I'd like to address the building a --version when no version exists regardless. Doing that would solve the problem of this issue outright with no additional changes.

Making global versions the default could then be either addressed separately and either kicked down the road, or rejected.

kbknapp added a commit that referenced this issue Oct 8, 2021
@kbknapp
imp(version): don't generate --version when version is empty
92903a5 
This commit changes the default behavior of clap to no longer generate a
`-V, --version` flag when no version information via `App::version` or
`App::long_version` has been provided.

This removes the requirement for `AppSettings::DisableVersionFlag`.
Additionally, `AppSettings::DisableVersionForSubcommands` is no longer
required since if no `--version` flag is built, no version flag can be
propagated down to subcommands.

Technically, clap still generates a version flag, however it is removed
prior to parsing if the user had not provided any version information.
This means that one can also still `App::mut_arg("version", |ver_flag|
...)` when changing the default version flag behavior is desired. Doing
so will "add version information" and cause clap to not remove this flag.

Relates to #2812

wip: removing DisableVersionFlag and DisableVersionForSubcommands
kbknapp added a commit that referenced this issue Oct 8, 2021
@kbknapp
tests: update tests to explicit version
44cf3e3 
This commit fixes up all tests to the new bevaior of no longer
generating a `-V, --version` flag unless the user has provided version
information.

Relates to #2812
kbknapp added a commit that referenced this issue Oct 8, 2021
@kbknapp
imp(version flag): no longer generates a version unconditionally
db7a472 
This commit changes the default behavior of clap to no longer generate a
`-V, --version` flag when no version information has been provided.

Version information can be provided via `App::version`,
`App::long_version`, or via `App::mut_arg("version", |_| ..)`. Using any
of the above is the only way to have clap auto-generate the version flag.

Technically, clap still generates a version flag, however it is removed
prior to parsing if the user has not provided any version information
via one of the mentioned methods.

Relates to [#2812](#2812)
kbknapp added a commit that referenced this issue Oct 8, 2021
@kbknapp
imp(AppSettings): removes no longer required DisableVersion[Flag|ForS…
8fa1340 
…ubcommands]

Because clap no longer auto-generates a version flag when there is no
version information, this removes the requirement for
`AppSettings::DisableVersionFlag`.

Additionally, `AppSettings::DisableVersionForSubcommands` is no longer
required since if no `--version` flag is built, no version flag can be
propagated down to subcommands.

Relates to #2812
kbknapp added a commit that referenced this issue Oct 8, 2021
@kbknapp
imp(AppSettings): removes no longer required DisableVersion[Flag|ForS…
5aec1a2 
…ubcommands]

BREAKING CHANGE

Because clap no longer auto-generates a version flag when there is no
version information, this removes the requirement for
`AppSettings::DisableVersionFlag`.

Additionally, `AppSettings::DisableVersionForSubcommands` is no longer
required since if no `--version` flag is built, no version flag can be
propagated down to subcommands.

Relates to #2812
@kbknapp kbknapp mentioned this issue Oct 8, 2021
Merged
kbknapp added a commit that referenced this issue Oct 8, 2021
@kbknapp
imp(version flag): no longer generates a version unconditionally
602b562 
This commit changes the default behavior of clap to no longer generate a
`-V, --version` flag when no version information has been provided.

Version information can be provided via `App::version`,
`App::long_version`, or via `App::mut_arg("version", |_| ..)`. Using any
of the above is the only way to have clap auto-generate the version flag.

Technically, clap still generates a version flag, however it is removed
prior to parsing if the user has not provided any version information
via one of the mentioned methods.

Relates to [#2812](#2812)
kbknapp added a commit that referenced this issue Oct 9, 2021
@kbknapp
imp(version flag): no longer generates a version unconditionally
7169c47 
This commit changes the default behavior of clap to no longer generate a
`-V, --version` flag when no version information has been provided.

Version information can be provided via `App::version`,
`App::long_version`, or via `App::mut_arg("version", |_| ..)`. Using any
of the above is the only way to have clap auto-generate the version flag.

Technically, clap still generates a version flag, however it is removed
prior to parsing if the user has not provided any version information
via one of the mentioned methods.

Relates to [#2812](#2812)
@bors bors bot closed this as completed in #2831 Oct 9, 2021
@joshtriplett
Copy link
Contributor Author

FWIW, another approach to solve this would be a different method to set the version, which only sets it for the top level command; effectively, combine setting version with setting a common option about --version.

That can be done post-3.0, so I don't think it needs to be a blocker.

@igotfr igotfr mentioned this issue Apr 10, 2022
Closed
@samkim-crypto samkim-crypto mentioned this issue Jun 8, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

No empty version clap-rs/clap
3 participants
@epage @joshtriplett @kbknapp