fix(help): Render partially-optional values with [] - #6454
Conversation
|
FYI we would prefer tests to be added in an earlier commit, reproducing the bad behavior, see our contrib docs as well as https://epage.github.io/dev/pr-style/#c-split |
934db25 to
46b650f
Compare
|
Thanks for the review, all three addressed. Rebased onto master and split into a test commit then the fix, so the fix commit's diff now shows only the rendering change. Pulled out On Happy to add a debug assert rejecting that combination if you want it, but it'd break anyone building that arg today so I kept it out of this PR. Let me know if you'd rather have it here or as a follow-up. |
Options with a value-count range render every value name as required, even for the values the parser will happily leave out.
An option declared with a value-count range, like `num_args(1..=2)` with
two value names, rendered as
--example <FOO> <BAR>
while the parser accepts `--example FOO`, so the help contradicted what
the parser would take. Render the value names past the minimum as
optional instead
--example <FOO> [BAR]
An option whose value is optional as a whole is bracketed by
`stylize_arg_suffix`, so its names stay unbracketed to avoid `[[name]]`.
Fixes clap-rs#4847
46b650f to
1ca5541
Compare
|
Thanks, all four addressed.
Still happy to add debug asserts for either unparseable combo — positional |
|
Thanks! Looks good! If you want to do a follow up to add to |
Fixes #4847.
While looking through the open help-rendering issues, I noticed this bug had already been tackled twice. #4910
went stale after accumulating conflicts, and #6376 was withdrawn because it lacked a regression test. The
underlying fix was straightforward, so I picked it up and completed it with the missing test coverage.
The issue was that options using a value-count range, such as
num_args(1..=2)with named values, renderedevery value name as required:
even though the parser accepts fewer values (for example,
--example FOO). This caused the help output tocontradict the parser's behavior. With this change, only the required values are rendered as required, while
additional values are shown as optional:
Value names beyond the minimum required count are now wrapped in
[]. Options whose entire value is optional(
min == 0) are already bracketed by the caller, so they continue to render as<name>to avoid producing[[...]].I also added unit tests covering ranged,
require_equals, and unbounded (1..=3) cases, along with a helpsnapshot test to prevent regressions. The full workspace test suite,
cargo fmt, andcargo clippyall pass.