fix(parser): flatten option groups automatically - #16
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
|
Codex review: needs maintainer review before merge. Reviewed August 12, 2026, 5:36 PM ET / 21:36 UTC. ClawSweeper reviewWhat this changesThe PR makes direct Merge readinessKeep open for normal merge handling: the focused parser normalization matches the existing signature model, and no actionable patch defect was found. This owner-authored PR is not eligible for automated cleanup. Priority: P2 Review scores
Verification
How this fits togetherCommander reflects property-wrapper command definitions into a command signature, then gives that metadata to the parser to interpret CLI arguments. Option groups are reusable nested metadata whose options and flags must reach parser lookup tables. flowchart LR
A[Command definition] --> B[Signature reflection]
B --> C[Option groups]
C --> D[Parser normalization]
D --> E[Argument lookup]
E --> F[Parsed command values]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the focused parser-boundary normalization after the remaining platform checks succeed. Do we have a high-confidence way to reproduce the issue? Yes, from source: pass Is this the best way to solve the issue? Yes. Normalizing once at AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a0bbecd4aad9. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 881a4f96ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| public init(signature: CommandSignature) { | ||
| self.signature = signature | ||
| self.signature = signature.flattened() |
There was a problem hiding this comment.
When an @OptionGroup type contains positional @Arguments, this unconditional flattening sends every parser through CommandSignature.flattened(), which appends grouped arguments after all parent arguments before validation. For a declaration such as an option group with @Argument input followed by a parent @Argument(parsing: .remaining) rest, parsing now throws invalidArgumentOrder("rest") because the variadic parent argument is moved ahead of the grouped argument; non-variadic grouped arguments are likewise interpreted in the wrong positional order. Either flatten only options/flags here or preserve declaration order for grouped positional arguments.
Useful? React with 👍 / 👎.
Summary
CommandParsernormalize nested@OptionGroupsignatures at its public ownership boundaryCommandSignature.flattened()implementationWhy
CommandSignature.describe(command)retains reusable groups as nested metadata. DirectCommandParsercallers could pass that public signature unchanged, but grouped options and flags were silently absent from parser lookup tables unless every caller remembered to flatten it first. The parser now owns that normalization, matching Commander's property-wrapper ergonomics and making already-flattened signatures remain idempotent.Testing
swift test: 43/43 passedswift test -c release: 43/43 passedswiftformat --lint .: 0/14 files require formattingswiftlint lint --config .swiftlint.yml: 0 violationsgit diff --check: passed