Add flag name suggestions for misspelled flags#4723
Open
simonfaltum wants to merge 5 commits intomainfrom
Open
Add flag name suggestions for misspelled flags#4723simonfaltum wants to merge 5 commits intomainfrom
simonfaltum wants to merge 5 commits intomainfrom
Conversation
When users misspell a flag (e.g., --outpu instead of --output), they now get a "Did you mean" suggestion. This extends Cobra's existing command suggestion behavior to flags using Levenshtein distance with a threshold of 2. Hidden and deprecated flags are excluded from suggestions. Both long flags (--flagname) and shorthand flags (-x) are handled. Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Collaborator
|
Commit: a0ef7d4
31 interesting tests: 14 flaky, 7 SKIP, 6 RECOVERED, 4 FAIL
Top 50 slowest tests (at least 2 minutes):
|
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Co-authored-by: Isaac
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
requested review from
andrewnester,
anton-107,
denik,
pietern and
shreyas-goenka
as code owners
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
shreyas-goenka
left a comment
shreyas-goenka
left a comment
There was a problem hiding this comment.
Note: This review was posted by Claude (AI assistant). Shreyas will do a separate, more thorough review pass.
Priority: HIGH — Should use typed errors instead of string parsing
MAJOR: String parsing error messages is fragile — use pflag.NotExistError typed errors
The PR parses error message strings with strings.HasPrefix(msg, "unknown flag: ") to extract flag names. However, pflag v1.0.10 (already a dependency) provides a structured pflag.NotExistError type with GetSpecifiedName() and GetSpecifiedShortnames() methods.
Should use: errors.As(err, &pflag.NotExistError{}) with the accessor methods instead of string parsing. This eliminates:
- Dependency on exact error message wording
- Brittle shorthand character extraction (
rest[0] != '\'') - The prefix constant definitions
MEDIUM: ShorthandDeprecated filtering too aggressive
In findClosestFlag, the check f.ShorthandDeprecated != "" excludes the entire flag from long-flag suggestions. ShorthandDeprecated only means the shorthand is deprecated, not the long flag itself. A flag with a deprecated shorthand should still appear in long-flag suggestions.
What looks good
- Clean separation in its own file, minimal integration point in root.go
- Levenshtein threshold of 2 matches Cobra's own command suggestion threshold
- Error wrapping with
%wpreserves error chain
Missing tests
- No integration test through Cobra's actual flag parsing
- No test for
ShorthandDeprecatedspecifically - No test for tie-breaking behavior for equidistant flags
The typed error approach is the main blocker for this PR.
…ated filtering, add tests Address PR review comments: - Replace string parsing (HasPrefix on error messages) with errors.As matching on pflag.NotExistError, using GetSpecifiedName() and GetSpecifiedShortnames() to extract flag info. - Fix findClosestFlag to no longer exclude flags with ShorthandDeprecated from long-flag suggestions. Only exclude deprecated shorthands from shorthand suggestions (in findClosestShorthand). - Add tests: integration through flagErrorFunc, ShorthandDeprecated filtering for both long and short suggestions, tie-breaking for equidistant flags. - Rewrite existing tests to use real Cobra flag parsing instead of hand-crafted error strings.
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
When users misspell a flag name (e.g.,
--outpuinstead of--output), they get a generic "unknown flag" error with no help. Cobra already suggests corrections for misspelled command names, but not flags.Changes
Before: Misspelled flags produce a generic "unknown flag" error with no guidance.
Now: The
flagErrorFuncsuggests the closest matching flag using Levenshtein distance (threshold of 2, matching Cobra's own suggestion threshold for commands).Both long flags (
--flagname) and shorthand flags (-x) are handled. Hidden and deprecated flags are excluded from suggestions. A small Levenshtein distance function is included inline (no new dependencies).Test plan
make checkspasses