Skip to content

Support custom FlagStringer for MutuallyExclusiveFlags - #2414

Open
SHIVANSHGARG07 wants to merge 5 commits into
urfave:mainfrom
SHIVANSHGARG07:fix-mutex-flags-stringer
Open

Support custom FlagStringer for MutuallyExclusiveFlags#2414
SHIVANSHGARG07 wants to merge 5 commits into
urfave:mainfrom
SHIVANSHGARG07:fix-mutex-flags-stringer

Conversation

@SHIVANSHGARG07

Copy link
Copy Markdown

What type of PR is this?

  • feature

What this PR does / why we need it:

Adds a Stringer field to MutuallyExclusiveFlags that lets callers
override how flags within the group are rendered in help output.
Previously, flags inside a mutex group could only use the package-level
FlagStringer, so there was no way to add group-specific context (e.g.
marking one of the flags as "required") to the help text.

  • flag.go: added the FlagStringerOverrider interface
    (SetStringer(FlagStringFunc)) that flags implement to opt in to a
    custom stringer.
  • flag_mutex.go: added Stringer FlagStringFunc field to
    MutuallyExclusiveFlags, plus propagateStringer() which pushes the
    group's stringer down to member flags (mirrors the existing
    propagateCategory() pattern used for Category).
  • flag_impl.go: FlagBase now stores an optional per-flag stringer
    and uses it in String() when set; implements SetStringer.
  • flag_bool_with_inverse.go: same treatment for BoolWithInverseFlag,
    which doesn't embed FlagBase and needed a separate implementation.
  • command_setup.go: calls grp.propagateStringer() alongside the
    existing grp.propagateCategory() call during command setup.
  • godoc-current.txt: regenerated to reflect the new public API.

Which issue(s) this PR fixes:

Fixes #2220

Special notes for your reviewer:

Followed the existing propagateCategory pattern for consistency.
BoolWithInverseFlag needed a standalone implementation since it
doesn't embed FlagBase.

Testing

Added TestMutuallyExclusiveFlags_PropagateStringer and
TestMutuallyExclusiveFlags_PropagateStringerNil in
flag_mutex_test.go. Ran the full suite with go test ./... — all
passing.

Release Notes

Added a `Stringer` field to `MutuallyExclusiveFlags`, allowing a custom
`FlagStringFunc` to override how flags within a mutually exclusive
group are displayed in help output.

Adds a Stringer field to MutuallyExclusiveFlags that lets callers
override how flags within the group are rendered in help output.
Flags opt in via the new FlagStringerOverrider interface.

Fixes urfave#2220
@SHIVANSHGARG07
SHIVANSHGARG07 requested a review from a team as a code owner August 18, 2026 16:29
FlagStringFunc is a func type and cannot be marshaled to JSON, which
broke the staticcheck SA1026 lint check via json.Marshal(cmd) in
existing tests. Tag the field with json:"-" to exclude it.

@dearchap dearchap left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall: Clean implementation following existing patterns. A few suggestions below.

@dearchap dearchap added the status/waiting-for-response Waiting for response from original requester label Sep 4, 2026
@dearchap

dearchap commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Suggestion 1: Rename FlagStringerOverrider to StringerSetter

The name FlagStringerOverrider is a tongue-twister. Consider StringerSetter — it is shorter, more idiomatic, and still clearly conveys intent. This would affect:

  • flag.go: interface definition
  • flag_mutex.go: type assertion in propagateStringer()
  • flag_mutex_test.go: no direct reference but indirectly
  • godoc-current.txt / godoc-v3.x.txt (after regeneration)
// Before
type FlagStringerOverrider interface {
    SetStringer(FlagStringFunc)
}

// After
type StringerSetter interface {
    SetStringer(FlagStringFunc)
}

This is a public API addition, so if you agree, now is the time to rename before merge.

@dearchap

dearchap commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Suggestion 2: Add test for non-implementing flags

The propagateStringer() loop uses a type assertion to silently skip flags that do not implement the interface. This is correct, but there is no test that verifies this behavior. Worth adding a test to lock in the skip behavior:

func TestMutuallyExclusiveFlags_PropagateStringerSkipsNonImplementor(t *testing.T) {
    customStringer := func(f Flag) string {
        return "custom:" + f.Names()[0]
    }

    // plainStringFlag is a minimal Flag impl that does NOT implement StringerSetter
    type plainStringFlag struct {
        StringFlag
    }

    grp := MutuallyExclusiveFlags{
        Stringer: customStringer,
        Flags: [][]Flag{
            {
                &plainStringFlag{StringFlag{Name: "plain"}},
            },
        },
    }

    // should not panic
    grp.propagateStringer()

    // stringer should NOT have been set — verify default behavior
    assert.NotEmpty(t, grp.Flags[0][0].String())
}

This ensures that the interface assertion skip path is covered and will not regress.

@dearchap

dearchap commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Suggestion 3: Verify make lint and make vet pass

The PR mentions go test ./... passes, but does not mention linting. Per the repo CI workflow, make lint (goimports check) and make vet also run. Please run these locally before merge:

make lint
make vet

This avoids surprises in CI, especially since the godoc files and source files were all modified.

@SHIVANSHGARG07

Copy link
Copy Markdown
Author

Thanks for the review! Addressed all three suggestions in the latest commit:

  1. Renamed FlagStringerOverrider → StringerSetter across flag.go, flag_mutex.go, and regenerated godoc-current.txt / testdata/godoc-v3.x.txt. Since this API hasn't been released yet, the rename is safe.
  2. Added TestMutuallyExclusiveFlags_PropagateStringerSkipsNonImplementor. Note: I used a standalone minimal Flag implementation instead of embedding StringFlag/FlagBase, since FlagBase now implements StringerSetter itself (that's the whole point of this PR), so embedding it would have made the test pass for the wrong reason.
  3. Re-ran make lint and make vet locally — both clean, along with make test, make generate, and make v3diff.

Also synced the branch with the latest main since it was flagged out-of-date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/waiting-for-response Waiting for response from original requester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Mutually exclusive flags in help output

2 participants