Skip to content

Feat:(issue_2392) Single-value arguments can be marked as required - #2393

Open
idelchi wants to merge 4 commits into
urfave:mainfrom
idelchi:required-single-arg
Open

Feat:(issue_2392) Single-value arguments can be marked as required#2393
idelchi wants to merge 4 commits into
urfave:mainfrom
idelchi:required-single-arg

Conversation

@idelchi

@idelchi idelchi commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • feature

What this PR does / why we need it:

  • args.go: adds a Required field to ArgumentBase, same as flags have. A missing required argument returns required argument "name" not set. Usage() now renders optional single args as [name], matching what {Type}Args already does.
  • args_test.go: adds TestSingleRequiredArg and TestChainedRequiredArgs. The TestArgUsage expectation for an optional arg changes from ia to [ia].
  • docs/v3/examples/arguments/advanced.md: short section on required arguments.
  • godoc-current.txt / testdata/godoc-v3.x.txt: regenerated.

A default Value does not satisfy Required, same as with flags.

Which issue(s) this PR fixes:

Fixes #2392

Special notes for your reviewer:

The help output change (optional single args rendering as [name]) is a separate commit and can be dropped if unwanted.

Testing

  • go test ./...
  • make gfmrun for the docs example
  • make generate + make v3approve for the godoc diff check

Release Notes

Single-value arguments can be marked as `Required`. Optional single-value arguments now render as `[name]` in help output.

@idelchi
idelchi requested a review from a team as a code owner July 20, 2026 20:07
@idelchi
idelchi force-pushed the required-single-arg branch from 7c2cf0a to 526922f Compare July 20, 2026 20:37
@dearchap

Copy link
Copy Markdown
Contributor

Thanks for the PR @idelchi! Nice, well-scoped feature. The semantics correctly mirror flags (a default Value doesn't satisfy Required, matching checkRequiredFlag's IsSet() check in command.go:410), the tests cover the interesting cases (default-not-satisfying, empty-string-provided, chained required args), and the docs gfmrun error spec matches the existing pattern. CI is green.

A few suggestions, in rough priority order:

  1. Error UX inconsistent with flags (main point). A missing required flag prints Incorrect Usage: ... plus help via checkAllRequiredFlags (command_run.go:346), returns a typed error (requiredFlagsErr), and reports all missing flags at once. A missing required arg returns a bare fmt.Errorf mid-parse (command_run.go:369-377): no help is shown, it isn't programmatically detectable, and it only reports the first missing arg. Consider pre-scanning arguments like the flags path (or at least wrapping in a typed error) so the behavior is parallel. (Minor: required argument "x" not set vs the flags' capitalized Required flag "x" not set.)

  2. Required-after-optional ordering is silently unsatisfiable. With [optional, required], the optional arg always consumes the token, so the required one can never be satisfied. This is inherent to positional args (same as multi-value Min), but a one-line note in the docs — or an ordering test — would help, since the docs currently don't mention it.

  3. Docs import ordering. The new example has "os" before "context" (pre-existing in the file too); goimports would reorder. gfmrun only compiles it, so it passes, but it's untidy.

  4. Minor test suggestion. Consider asserting Destination/Get() are untouched on the missing-required path, and a help-output test that renders [sa] vs sa for the required case — TestArgUsage covers Usage() but not the full help line.

I'd be happy to merge once #1 is decided on (align with the flags error path, or explicitly accept the divergence).

@idelchi
idelchi force-pushed the required-single-arg branch from 526922f to 18fa3fa Compare August 15, 2026 23:31
@idelchi
idelchi force-pushed the required-single-arg branch from 18fa3fa to 11b3f89 Compare August 15, 2026 23:36

@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.

Issues to address

1. Duplicate enforcement with divergent error handling (moderate)

Required is checked in two places:

  • checkRequiredArguments (command.go:457) — pre-parse path that prints Incorrect Usage: ... + help and sets isInError.
  • ArgumentBase.Parse (args.go) — the arg-parsing loop only calls OnUsageError + handleExitCoder, with no help output and no isInError.

The Parse-level check is effectively dead code for the documented (required-first) ordering and only fires when a multi-value/variadic arg precedes a required arg. When it does fire, the error handling differs from the primary path (no help, different exit handling). Consider dropping one, or unifying the handling so both paths behave identically.

2. Index-based counting breaks with multi-value args (command.go:465)

index >= providedArguments assumes each declared argument consumes exactly one positional. I reproduced with:

Arguments: []Argument{
    &StringArgs{Name: "rest", Min: 0, Max: -1},
    &StringArg{Name: "required", Required: true},
}
cmd.Run(ctx, []string{"foo", "a", "b", "c"}) // → error: Required argument "required" not set

3 args are provided but the variadic consumes them all, so the arg genuinely receives no value — but the error is misleading. The docs do tell users to declare required args before optional/multi-value args, and the Parse backstop catches it, so acceptable — but worth a comment or an error message that hints at the ordering rule.

3. Missing builtInHelp || isCompletionCommand guard (command.go:457)

checkAllRequiredFlags (command.go:424) skips both; checkRequiredArguments doesn't. Currently harmless because --help short-circuits earlier via checkHelp() (command_run.go:215) and the built-in completion subcommands have no Arguments, but it's fragile if help/completion flow changes. Add the same guard for parity.

Minor / nits

  • requiredArgument interface uses unexported methods (name(), required()), diverging from flags' public RequiredFlag pattern (flag.go:116). Custom third-party Argument implementations can never participate — fine, but inconsistent.
  • Usage() now renders optional single args as [name] — a visible change to existing help output. It's consistent with ArgumentsBase and covered by the updated TestArgUsage, so I'd keep it, but it deserves the explicit release-note mention (which it has).
  • Empty string ("") counts as "provided" — consistent with --flag "", fine.

Net: reasonable to merge after addressing #1 and considering #2/#3.

@idelchi

idelchi commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review @dearchap Take a look if it's okay now

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.

No way to mark a single-value argument as required

2 participants