Skip to content

fix: allow DefaultCommand to handle its own flags#2322

Open
lihan3238 wants to merge 6 commits into
urfave:mainfrom
lihan3238:fix-2249
Open

fix: allow DefaultCommand to handle its own flags#2322
lihan3238 wants to merge 6 commits into
urfave:mainfrom
lihan3238:fix-2249

Conversation

@lihan3238
Copy link
Copy Markdown

Fixes #2249

Summary

  • When DefaultCommand is set, flags defined on the default subcommand now work without specifying the subcommand name
  • Previously, unknown flags were rejected during parent command parsing before default command routing
  • Pass unknown flags through as positional args when DefaultCommand is set

Test plan

  • Added regression test TestDefaultCommandWithSubcommandFlags
  • All existing DefaultCommand tests pass
  • Full test suite passes

When DefaultCommand is set, flags defined on the default subcommand
should work even without specifying the subcommand name. Previously,
unknown flags were rejected during parent command parsing before the
default command routing could occur.

Pass unknown flags through as positional args when DefaultCommand is
set, allowing the default command to handle them correctly.

Fixes urfave#2249
@lihan3238 lihan3238 requested a review from a team as a code owner May 8, 2026 19:18
lihan3238 added 2 commits May 12, 2026 17:24
Covers the shortOptionHandling path in parseFlags where unknown short
flags are passed through when DefaultCommand is set, improving codecov
patch coverage from 50% to 100%.
Comment thread command_parse.go Outdated
Comment thread command_test.go Outdated
Comment thread command_test.go Outdated
- Only pass unknown flags to DefaultCommand when the entire flag is
  unknown (index == 0), not when hitting unknown chars mid-split
- Move assertion outside action callback with actionExecuted guard
- Rename confusing flag name 'bar' to 'value' to avoid ambiguity with
  the 'bar' value used for the 'foo' flag
@lihan3238
Copy link
Copy Markdown
Author

@dearchap Thanks for the review! I've addressed all three points in the latest push:

  1. L215 — Added index == 0 guard so only a fully-unknown flag triggers the DefaultCommand passthrough, not an unknown char mid-split
  2. L5739 — Moved the assertion outside the action callback using an actionExecuted guard variable
  3. L5758 — Renamed bar flag to value to avoid confusion with the "bar" value

Comment thread command_test.go
Name: "run",
Usage: "run the app",
Action: func(ctx context.Context, cmd *Command) error {
if cmd.String("foo") != "baz" {
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.

if the action isnt run for some reason (due to fault args or something) then this check wont run. Add the actionRun flag as above

@lihan3238
Copy link
Copy Markdown
Author

@dearchap Good catch - added actionRun guard to both TestDefaultCommandWithShortFlag and TestDefaultCommandWithShortFlagHandling in 68d38c4. The assertion now runs outside the action callback, matching the pattern in TestDefaultCommandWithSubcommandFlags.

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.

DefaultCommand don't work correctly with Flags

2 participants