feat(argv): a flag whose value may be left off - #969
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR adds help-only metadata for flag values displayed as optional while preserving the existing parser grammar.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (5): Last reviewed commit: "fix(derive): refuse a value_optional not..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
2b1fb98 to
4325939
Compare
…it too Two things reported on #969, both about the same silence. A positional accepted `#[usage(value_optional)]` and dropped it: only the flag metadata emitter reads the field. A positional's brackets already come from its type — `Option<T>` renders `[NAME]` and `T` renders `<NAME>` — so there was nothing to declare, and the page went on saying the opposite of what was asked for. Refused now, beside the `bool`/`count` case, with the message pointing at the type. The clap dialect dropped the fact with no `skipped.note`, so the fidelity report went quiet about something it exists to report. Emitted rather than noted, because clap *can* say it: `num_args = 0..=1`, the same word this file already uses for a variadic's bounds. A note would have reported a limitation clap does not have. Both mutation-checked.
|
Both valid, both fixed in Positional attributes silently discarded — correct, and worse than "discarded": a positional's brackets already come from its type, so the page went on saying the opposite of what was asked for. Refused now, beside the Clap shadow drops optional values — also correct, and the fix is to emit rather than note. clap can say this: Both mutation-checked — disabling either guard fails its named test. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 17a79ed. Configure here.
`arg "[BUMP]" required=#false` inside a flag says the value is optional, and
usage-argv had nowhere to put it: `Flag` carries `takes_value: bool` and
nothing finer. So pitchfork's `--bump` rendered `<BUMP>` where its own spec
says `[BUMP]`, on three of its pages.
Measured before deciding where it belongs. usage-lib's parser refuses a bare
`--bump` exactly as it refuses a bare `--port`:
["--bump"] -> ERR Invalid flag `--bump`: requires an argument
["--bump", "5"] -> OK bump="5"
So this binds nothing differently — it changes the brackets. That puts it in
`FlagMeta`, which a parse never reads, rather than in `Flag`, which is on the
hot path and gains no field.
Declared rather than inferred: `Option<String>` already says the *flag* is
optional and says nothing about its value, so `#[usage(value_optional)]` is
the only way to state it. Refused where the field takes no value, like
`choices` and `value_enum` beside it.
The emitted KDL writes both halves — `arg "[BUMP]" required=#false` — because
usage-lib reads the attribute as well as the name, so square brackets alone
round-trip as required.
With this and the version banner, six of the seven jdx CLIs now render
byte-identically to usage-lib across every command: hk, fnox, aube, tak,
communique and mise. pitchfork has two long pages left, from a blank-line
difference that is its own change.
…clap's loss Two rounds of feedback on #969, and the second round corrected the first fix. **The guard was testing the wrong thing.** `is_arg` is only true where `arg` was written, so `#[usage(value_optional)] out: Option<String>` — a positional by default, with no `long`, `short` or `arg` — still compiled and was dropped. It now tests `is_flag`, which covers both spellings of a positional. Only a flag's value has a say in this: a positional's brackets come from its type, so the page went on saying the opposite of what was declared. **And clap must not be told.** The first fix emitted `num_args = 0..=1`, reasoning that clap can express an optional value and a `skipped.note` would report a limitation it does not have. That was wrong, and Bugbot said so plainly: `value_optional` is help-only — usage-lib's parser refuses a bare `--bump` exactly as it refuses a bare `--port` — so declaring it in clap makes the clap shadow accept a line the usage shadow rejects. The pair exists to be one grammar told to two frameworks, and a fixture that quietly changes the grammar is worse than one that reports a gap. So it is a note after all, and a truthful one: clap cannot print `[BUMP]` without also accepting `--bump` on its own. Both mutation-checked.
17a79ed to
9560b0e
Compare
|
Second round, and the second finding corrects my first fix. Both addressed in Implicit positionals bypass validation — right, and the guard was testing the wrong thing. #[derive(Cli)]
#[usage(bin = "ex")]
struct Ex {
#[usage(value_optional)] // no long, no short, no arg
out: Option<String>,
}compiled clean and emitted a spec with no trace of the declaration. The guard now tests Clap shadow changes parse semantics — you're right and I was wrong. I emitted Reverted to a note, and the note is truthful in a way my first reading missed: clap cannot print The test now asserts both halves — no Thanks for pushing back on that one; the first version would have made the perf comparison subtly dishonest. AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable. |
Rebased onto main, which changed both halves of this PR's premise. #969 fixed the optional flag value independently — `FlagMeta::value_optional`, the opposite polarity of the field this branch was carrying, so that commit is dropped and the builder speaks main's spelling. And #972 widened the parity gate from mise alone to all seven jdx CLIs, so "one CLI cannot cover this" is no longer the argument. The argument is better now, and measured rather than asserted. Across all 809 value-taking flags in mise and the fleet, three of the four flag/value bracket pairings appear — 796, 8 and 5 — and `<--jobs [n]>` appears nowhere, nor does a value carrying a `default`, which relaxes the value's brackets by another route. The pairing #969 fixed had exactly five instances, all in pitchfork and aube, which is the whole reason it was visible at all. Seven real CLIs still leave shapes uncovered, and the ones left are not exotic. Also recorded why the gate cannot see the divergence this corpus found: six of the seven CLIs declare a top-level `usage` synopsis, but `xtask gen-shadow` does not carry the node into the shadow, so every shadow's `Spec::usage` is `None` and both sides render the generated line. The corpus builds usage-argv's tables from KDL directly and sees it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
arg "[BUMP]" required=#falseinside a flag says the value is optional, andusage-argv had nowhere to put it:
Flagcarriestakes_value: boolandnothing finer. So pitchfork's
--bumprendered<BUMP>where its own specsays
[BUMP], on three of its pages.Measured before deciding where it belongs. usage-lib's parser refuses a bare
--bumpexactly as it refuses a bare--port:So this binds nothing differently — it changes the brackets. That puts it in
FlagMeta, which a parse never reads, rather than inFlag, which is on thehot path and gains no field.
Declared rather than inferred:
Option<String>already says the flag isoptional and says nothing about its value, so
#[usage(value_optional)]isthe only way to state it. Refused where the field takes no value, like
choicesandvalue_enumbeside it.The emitted KDL writes both halves —
arg "[BUMP]" required=#false— becauseusage-lib reads the attribute as well as the name, so square brackets alone
round-trip as required.
With this and the version banner, six of the seven jdx CLIs now render
byte-identically to usage-lib across every command: hk, fnox, aube, tak,
communique and mise. pitchfork has two long pages left, from a blank-line
difference that is its own change.
Stack created with GitHub Stacks CLI • Give Feedback 💬
Note
Low Risk
Help and spec emission only; parsing behavior is explicitly unchanged and covered by conformance tests.
Overview
Adds
value_optionalso flags whose spec saysarg "[NAME]" required=#falseshow--flag [NAME]in help and KDL instead of<NAME>, without changing parsing (bare--flagstill requires a value).#[usage(value_optional)]on derive flags flows throughFlagMeta, help rendering, andto_kdl(brackets plusrequired=#falsefor round-trip). The attribute is rejected on positionals and valueless flags.xtask shadow maps optional spec args to
value_optionalfor the usage dialect and records a clap limitation when help-only optionality cannot be expressed without diverging grammars.Conformance tests lock help parity with usage-lib and emitted KDL.
Reviewed by Cursor Bugbot for commit 9560b0e. Bugbot is set up for automated code reviews on this repo. Configure here.
AI-assisted — Tool: Claude Code; model: anthropic/claude-opus-5; version: unavailable.