fix: don't panic when -h shorthand is already in use#2367
fix: don't panic when -h shorthand is already in use#2367dimitar-trifonov wants to merge 3 commits intospf13:mainfrom
Conversation
InitDefaultHelpFlag previously always attempted to register the default help flag with shorthand "h". If a command (or its persistent flags) already used "h" for another flag, pflag would panic due to the duplicate shorthand. Avoid the panic by checking whether shorthand "h" is already taken before adding the default help flag. When "h" is unavailable, register the help flag without a shorthand instead. Refs: spf13#2359 Signed-off-by: Dimitar Trifonov <trifonov.dimitar@gmail.com>
|
|
This would be a new behaviour. Currently, Could we adapt this PR to keep the panic but be smarter about it? We need to be able to know what the real name of the help flag (after translation) so that we can check that name to know if the help flag has already been added or not. |
The previous change avoided a panic by falling back to a long-only help flag when shorthand "h" was already in use. Reviewer feedback noted this would be a behavior change: -h is intentionally reserved for help, and the panic acts as an early signal of a developer conflict. Restore the panic on shorthand conflicts, but make it explicit and clearer by detecting an existing -h flag and panicking with a targeted message. Update the test to assert the reserved-shorthand behavior. Refs: spf13#2367 Context: reviewer feedback on PR
The previous change avoided a panic by falling back to a long-only help flag when shorthand "h" was already in use. Reviewer feedback noted this would be a behavior change: -h is intentionally reserved for help, and the panic acts as an early signal of a developer conflict. Restore the panic on shorthand conflicts, but make it explicit and clearer by detecting an existing -h flag and panicking with a targeted message. Update the test to assert the reserved-shorthand behavior. Refs: spf13#2367 Context: reviewer feedback on PR
dimitar-trifonov
force-pushed
the
issue-2359-no-panic-when-h-is-in-use
branch
from
adff99b to
f4b4aaf
Compare
The previous change avoided a panic by falling back to a long-only help flag when shorthand "h" was already in use. Reviewer feedback noted this would be a behavior change: -h is intentionally reserved for help, and the panic acts as an early signal of a developer conflict. Restore the panic on shorthand conflicts, but make it explicit and clearer by detecting an existing -h flag and panicking with a targeted message. Update the test to assert the reserved-shorthand behavior. Refs: spf13#2359 Context: reviewer feedback on PR spf13#2367
dimitar-trifonov
force-pushed
the
issue-2359-no-panic-when-h-is-in-use
branch
from
f4b4aaf to
43ac170
Compare
Context: golangci-lint gosec G703 (user-provided BASH_COMP_DEBUG_FILE)
| usage += name | ||
| } | ||
| if f := c.Flags().ShorthandLookup("h"); f != nil { | ||
| panic(fmt.Sprintf("shorthand '-h' is reserved for the help flag, but is already used by flag '%s'", f.Name)) |
There was a problem hiding this comment.
I would mention that InitDefaultFlag cannot be used because -h is already registered by f.Name
There was a problem hiding this comment.
@ccoVeille you are correct: with the latest change we keep the "-h is reserved for help" behavior.
If a developer registers -h for another flag (e.g. ayuda), Cobra now panics early with a clearer message:
panic: shorthand '-h' is reserved for the help flag, but is already used by flag 'ayuda'
This is also covered by a unit test:
TestInitDefaultHelpFlagPanicsWhenHShorthandAlreadyUsed
cc: @marckhouzam
4 participants
InitDefaultHelpFlag previously always attempted to register the default help flag with shorthand "h". If a command (or its persistent flags) already used "h" for another flag, pflag would panic due to the duplicate shorthand.
Avoid the panic by checking whether shorthand "h" is already taken before adding the default help flag. When "h" is unavailable, register the help flag without a shorthand instead.
Refs: #2359