Skip to content

Use prefixFor so flag suggestions agree with help on rune count - #2448

Merged
Juneezee merged 2 commits into
urfave:mainfrom
januththedev:fix/suggest-flag-rune-prefix
Sep 29, 2026
Merged

Juneezee merged 2 commits into
urfave:mainfrom
januththedev:fix/suggest-flag-rune-prefix

Conversation

@januththedev

Copy link
Copy Markdown

"Did you mean" suggests --é for a flag that help renders as -é

Description

suggestions.go, in suggestFlag:

if len(suggestion) == 1 {
    suggestion = "-" + suggestion
} else if len(suggestion) > 1 {
    suggestion = "--" + suggestion
}

len() counts bytes, but a single-rune flag name can be multi-byte. Everywhere else in the library the same "one dash vs two dashes" decision is made by rune count — prefixFor in docs.go:11-19:

if utf8.RuneCountInString(name) == 1 { prefix = "-" } else { prefix = "--" }

Why it's wrong

Help output and the "Did you mean …?" hint disagree about the same flag. A BoolFlag{Name: "é"} is advertised in help as -é (via prefixFor) but was suggested as --é, so the user is told to type a spelling the help text never showed them.

Reproduction (real output, before the fix)

=== RUN   TestSuggestFlagMultibyteRunePrefix/single-rune-multibyte
    suggestions_test.go:84:
        Error: Not equal:
            expected: "-é"
            actual  : "--é"
--- FAIL: TestSuggestFlagMultibyteRunePrefix (0.00s)
    --- PASS: TestSuggestFlagMultibyteRunePrefix/single-rune-ascii (0.00s)
    --- FAIL: TestSuggestFlagMultibyteRunePrefix/single-rune-multibyte (0.00s)

Note the first assertion in that subtest passes: help really does render -é. Only the suggestion says --é.

End-to-end on unmodified HEAD:

$ app --éé
Incorrect Usage: flag provided but not defined: -éé

Did you mean "--é"?

After the fix the same invocation prints Did you mean "-é"?.

The fix

-	if len(suggestion) == 1 {
-		suggestion = "-" + suggestion
-	} else if len(suggestion) > 1 {
-		suggestion = "--" + suggestion
+	if len(suggestion) == 0 {
+		return ""
 	}
 
-	return suggestion
+	// Use the same rune-counting rule that drives the help output
+	// (prefixFor) so that a single-rune name is suggested as a short flag
+	// regardless of how many bytes that rune is encoded in.
+	return prefixFor(suggestion) + suggestion

Reusing prefixFor is the point: the correct helper already exists in the same package, so this removes the duplicated (and wrong) decision rather than adding a third one.

Tests

  • TestSuggestFlagMultibyteRunePrefix (table-driven, 2 subtests) asserts the suggestion agrees with stringifyFlag's own prefix.
  • TestSuggestFlagMultibyteRuneFromError asserts the exact Did you mean "-é"? string from the end-to-end path.
count
Pre-existing failures on unmodified HEAD 0
Baseline 1329 PASS, 0 FAIL, 6 SKIP
After 1333 PASS, 0 FAIL, 6 SKIP (+4 = my new tests)

go build ./... and go vet ./... clean. No public API change — suggestFlag is unexported — so the v3diff godoc gate needs no regeneration.

Upstream status

Nothing open covers this. I deliberately avoided the areas already claimed: #2443 (non-last value flag in a short group), #2396 (duplicate flag names / help clash), #2422 (inherited persistent flags in help), #2382, #2261, #2376/#2384, #2209/#2212, #2342/#2379. Issue #2320 is a question about slice-flag help duplication, unrelated.

Other candidates I left alone

  • FlagBase.count is never reset by PreParse, so re-running the same *Command makes OnlyOnce fail and cmd.Count() double-count on the second invocation (flag_impl.go:79,185).
  • errRequiredFlags.Error() wraps all names in one quote pair, so it renders Required flags "a, b" not set instead of "a", "b" (errors.go:60-61).
  • SuggestFlag includes hidden flags in suggestions, which contradicts their purpose (suggestions.go:109).

@januththedev
januththedev requested a review from a team as a code owner September 28, 2026 18:47
Replace the two new tests and the flagStringForTest helper with a case
in TestSuggestFlag. prefixFor with a single-rune Unicode name is tested
by TestCommand_SingleRuneUnicodeFlag, and the suggestFlagFromError path
is covered by TestSuggestFlagFromError.

Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>

@Juneezee Juneezee left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. Reusing prefixFor is the right call, and it makes the suggestion match the help output. I pushed a commit that trims the tests to one case in TestSuggestFlag: bc3e336

I noticed that you appear to be using an AI agent to open dozens of issues and PRs across many projects over the past week. At least one project has already rejected one of them under its AI policy: pallets/click#3879

urfave/cli has no rule against AI-assisted contributions, but every PR takes maintainer time, and this one needed a follow-up commit before it could be merged. Before opening more PRs here, please make sure you have read and understood the change yourself, keep the tests to what the change needs, and keep the description short and focused on the change. PRs that look unreviewed may be closed without review.

@Juneezee
Juneezee merged commit 7389061 into urfave:main Sep 29, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants