feat(args): support optional positional arguments - #572
Conversation
|
Warning Review limit reached
More reviews will be available in 14 minutes and 26 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR implements optional positional arguments for the Gunshi CLI framework. The ChangesOptional Positional Arguments Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
@gunshi/bone
@gunshi/combinators
@gunshi/definition
@gunshi/docs
gunshi
@gunshi/plugin
@gunshi/plugin-completion
@gunshi/plugin-dryrun
@gunshi/plugin-global
@gunshi/plugin-i18n
@gunshi/plugin-renderer
@gunshi/resources
@gunshi/shared
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/docs/src/api/combinators/functions/positional.md (1)
35-41: ⚡ Quick winEnhance documentation to explain optional positional arguments.
The examples now include
unrequired(positional())to demonstrate optional positionals, which is excellent. However, the main description text (lines 12-15 and 52-55) does not mention that positional arguments can be made optional. For a newly introduced feature, users would benefit from a brief explanation in the description, such as:"By default, positional arguments are required. Wrap with
unrequired()to make them optional."Consider also adding a cross-reference link to the
unrequired()combinator documentation to help users discover the full API. As per coding guidelines, documentation should be updated when adding new features, and explaining the optional behavior (not just showing an example) better serves that goal.Also applies to: 69-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/docs/src/api/combinators/functions/positional.md` around lines 35 - 41, The docs for positional arguments need a short explanatory sentence noting that positional() arguments are required by default and can be made optional by wrapping them with unrequired(positional()); add this sentence near the main description (where positional() is introduced) and again where examples are discussed, and add a cross-reference link to the unrequired() combinator docs so readers can learn its behavior and return type changes.Source: Coding guidelines
packages/docs/src/guide/essentials/auto-usage.md (1)
238-238: 💤 Low valueConsider breaking this long sentence into multiple sentences for readability.
The current sentence is 47 words long and packs multiple concepts (required vs optional, bracket notation, multiple values, mixed requirements). While technically correct, splitting it into 2-3 shorter sentences would improve clarity for readers.
✏️ Suggested rewrite for improved readability
-Positional arguments are displayed with clear, descriptive names that indicate their purpose. Required positional arguments use angle brackets (e.g., `<resource>`), while optional positional arguments use square brackets (e.g., `[<query>]`). Multiple positional arguments are shown as `[<file> ...]`, or `<file> [<file> ...]` when at least one value is required. +Positional arguments are displayed with clear, descriptive names that indicate their purpose. Required positional arguments use angle brackets (e.g., `<resource>`), while optional positional arguments use square brackets (e.g., `[<query>]`). When multiple values are accepted, optional arguments are shown as `[<file> ...]`, or `<file> [<file> ...]` when at least one value is required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/docs/src/guide/essentials/auto-usage.md` at line 238, Split the long explanatory sentence in the "Positional arguments are displayed..." paragraph into 2–3 shorter sentences for clarity: first state the general rule that required positional arguments use angle brackets (e.g., `<resource>`) and optional ones use square brackets (e.g., `[<query>]`), then add a separate sentence explaining how multiple values are shown (e.g., `[<file> ...]`) and a final short sentence (if needed) describing mixed requirements (`<file> [<file> ...]`) to keep each concept focused and readable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/docs/src/api/docs.json`:
- Around line 1903-1909: The documentation for ArgSchema.default currently
claims "positional/custom" can be "any appropriate default" which contradicts
the declared type "string | boolean | number"; update the prose in the "default"
property description (the block under the "default" property) to restrict
positional/custom defaults to the declared primitive union (string, boolean,
number) or, if non-primitive defaults are actually supported, change the
declared "type" to reflect that; edit the description text where it mentions
positional/custom and any other occurrences (lines around the same "default"
blocks at the other noted locations) so the wording and the type union are
consistent with ArgSchema.default.
---
Nitpick comments:
In `@packages/docs/src/api/combinators/functions/positional.md`:
- Around line 35-41: The docs for positional arguments need a short explanatory
sentence noting that positional() arguments are required by default and can be
made optional by wrapping them with unrequired(positional()); add this sentence
near the main description (where positional() is introduced) and again where
examples are discussed, and add a cross-reference link to the unrequired()
combinator docs so readers can learn its behavior and return type changes.
In `@packages/docs/src/guide/essentials/auto-usage.md`:
- Line 238: Split the long explanatory sentence in the "Positional arguments are
displayed..." paragraph into 2–3 shorter sentences for clarity: first state the
general rule that required positional arguments use angle brackets (e.g.,
`<resource>`) and optional ones use square brackets (e.g., `[<query>]`), then
add a separate sentence explaining how multiple values are shown (e.g., `[<file>
...]`) and a final short sentence (if needed) describing mixed requirements
(`<file> [<file> ...]`) to keep each concept focused and readable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dce5cba9-ccac-4882-9a52-dbd6682f1c54
⛔ Files ignored due to path filters (2)
packages/plugin-renderer/src/__snapshots__/usage.test.ts.snapis excluded by!**/*.snappnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
packages/docs/package.jsonpackages/docs/src/api/combinators/functions/positional.mdpackages/docs/src/api/combinators/functions/unrequired.mdpackages/docs/src/api/docs.jsonpackages/docs/src/guide/essentials/auto-usage.mdpackages/docs/src/guide/essentials/declarative.mdpackages/docs/src/guide/experimentals/parser-combinators.mdpackages/gunshi/package.jsonpackages/gunshi/src/cli.test.tspackages/gunshi/src/combinators.test-d.tspackages/gunshi/src/combinators.test.tspackages/plugin-renderer/README.mdpackages/plugin-renderer/src/usage.test.tspackages/plugin-renderer/src/usage.ts
Deploying gunshi with
|
| Latest commit: |
31916bf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://666bba02.gunshi.pages.dev |
| Branch Preview URL: | https://feat-optional-positional-arg.gunshi.pages.dev |
1 participant
Summary
Update Gunshi to args-tokens v0.26.0 and expose optional positional argument support.
The renderer now marks optional/defaulted positional args with square brackets, tests cover raw schemas and
unrequired(positional()), and docs/API references describe required/default behavior and usage output.close #571
Summary by CodeRabbit
New Features
<...>), optional positionals use square brackets ([<...>]).Documentation
Tests
Chores
args-tokensdependency to^0.26.0.