feat: resolve positional argument placeholder values with argument schema - #111
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces first-class support for positional arguments (placeholders) in the CLI tool. The core logic for argument parsing is adjusted to recognize and validate positional arguments, including within subcommands. Usage rendering is enhanced to display positional arguments distinctly from optional arguments, with new helper functions and test cases added to ensure correct parsing and output. Locale files and constants are updated to support the new "ARGUMENTS" section for both English and Japanese. Dependency versions are also incremented, but no functional changes are made outside of the positional argument feature. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Parser
participant Renderer
User->>CLI: Execute command with positional and optional args
CLI->>Parser: Parse arguments (with skipPositional logic)
Parser-->>CLI: Parsed args (positional + optional)
CLI->>Renderer: Render usage/help (with positional args section)
Renderer-->>CLI: Usage/help output
CLI-->>User: Display output/errors/usage
Assessment against linked issues
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@gunshi/bone
@gunshi/definition
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
🔭 Outside diff range comments (1)
src/renderer/usage.ts (1)
241-247:⚠️ Potential issue
hasAllDefaultOptionswrongly counts positional arguments
generateOptionsSymbols()decides between[OPTIONS]and<OPTIONS>by callinghasAllDefaultOptions().
With the new positional–argument support, every positional arg lacksdefault, so mixed commands (foo,--bar) will always show<OPTIONS>even when every optional arg has a default.-function hasAllDefaultOptions<A extends Args>(ctx: CommandContext<A>): boolean { - return !!(ctx.args && Object.values(ctx.args).every(arg => arg.default)) -} +function hasAllDefaultOptions<A extends Args>(ctx: CommandContext<A>): boolean { + return !!( + ctx.args && + Object.values(ctx.args) + .filter(arg => arg.type !== 'positional') // ignore positionals + .every(arg => arg.default !== undefined) + ) +}
🧹 Nitpick comments (5)
package.json (1)
111-113: Use a conservative range specifier for production dependencies
args-tokenswas bumped from^0.17.0to^0.17.1.
Because the caret operator allows future minor/patch releases, builds may silently consume a later, potentially breaking, version.
If deterministic builds are important (CI, Docker image, etc.) consider pinning ("0.17.1") or at least using a tilde (~0.17.1).
Dev-dependencies are less critical, but production-time deps affect every downstream consumer.src/renderer/usage.ts (1)
381-387: Minor: unnecessary tuple indices & template wrapperA micro-clean-up: the unused parameters (
_,__) and outer template literal add noise.-function generatePositionalSymbols<A extends Args>(ctx: CommandContext<A>): string { - return hasPositionalArgs(ctx) - ? `${getPositionalArgs(ctx) - .map(([name, _], __) => `<${name}>`) - .join(' ')}` - : '' -} +function generatePositionalSymbols<A extends Args>(ctx: CommandContext<A>): string { + if (!hasPositionalArgs(ctx)) return '' + return getPositionalArgs(ctx) + .map(([name]) => `<${name}>`) + .join(' ') +}Purely cosmetic – skip if you prefer brevity.
src/renderer.test.ts (1)
336-340: Copy-paste description typo
bazis declared as a positional argument but its description reads"The bar argument".- description: 'The bar argument' + description: 'The baz argument'src/cli.test.ts (2)
679-682: Assertion too strict – prefertoContainor snapshotError strings are user-visible and may change (word-wrapping, locale, punctuation).
A stricttoEqualwill break the test for any minor wording tweak.
Consider:-expect(stdout).toEqual( - `Optional argument '--foo' should be chosen from 'enum' ["a", "b", "c"] values` -) +expect(stdout).toContain(`'--foo'`) +expect(stdout).toContain(`["a", "b", "c"]`)Keeps intent while reducing brittleness.
684-686: Avoidasynccallbacks indescribeblocksVitest executes the callback synchronously to collect tests; returning a promise is ignored and may mask accidentally-awaited setup.
Drop theasynckeyword:-describe('positional arguments', async () => { +describe('positional arguments', () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/__snapshots__/cli.test.ts.snapis excluded by!**/*.snapsrc/__snapshots__/generator.test.ts.snapis excluded by!**/*.snapsrc/__snapshots__/renderer.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
package.json(2 hunks)src/cli.test.ts(1 hunks)src/cli.ts(1 hunks)src/constants.ts(1 hunks)src/locales/en-US.json(1 hunks)src/locales/ja-JP.json(1 hunks)src/renderer.test.ts(5 hunks)src/renderer/usage.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/cli.test.ts (4)
src/utils.ts (1)
log(88-90)test/utils.ts (1)
defineMockLog(15-22)src/definition.ts (2)
Args(20-20)define(27-29)src/cli.ts (1)
cli(22-85)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
src/locales/ja-JP.json (1)
6-6: Translation looks goodThe addition of "ARGUMENTS": "引数" correctly provides the Japanese translation for the new "ARGUMENTS" term, maintaining consistent localization across the application.
src/constants.ts (1)
70-70: Appropriate addition to resource keysAdding 'ARGUMENTS' to the COMMAND_BUILTIN_RESOURCE_KEYS array is well-placed between 'COMMANDS' and 'OPTIONS', creating a logical sequence of UI sections for command rendering.
src/locales/en-US.json (1)
6-6: Consistent localization additionThe English localization entry for "ARGUMENTS" matches the style of other section headings and provides the necessary localization for the new arguments section.
src/cli.ts (1)
39-41: Good conditional handling of positional argumentsThe addition of the
skipPositionaloption with conditional logic effectively handles positional arguments differently based on whether subcommands are present:
- When subcommands exist (
skipPositional: 0), the first positional argument (the subcommand name) is skipped from regular positional argument processing- When no subcommands exist (
skipPositional: -1), all positional arguments are processed normallyThis implementation correctly resolves positional argument values according to the command structure, aligning with the PR objective.
Deploying gunshi with
|
| Latest commit: |
dc824c0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://93af242e.gunshi.pages.dev |
| Branch Preview URL: | https://feat-positional-args.gunshi.pages.dev |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/renderer/usage.ts (1)
369-372: 👍 Description fallback implemented – matches earlier reviewThe two-level fallback (
ctx.translate(..) || schema.description || '') mirrors the logic already used for optional arguments and resolves the “blank description” issue noted in the previous review.
🧹 Nitpick comments (1)
docs/guide/essentials/declarative-configuration.md (1)
163-196: Minor punctuation / style tweaks in the Positional Arguments sectionThere are a couple of small wording / punctuation issues that slightly interrupt the flow:
- Line 190 – consider adding a comma after flags (“…cannot be truly optional like named flags, …”).
- Line 193 – a leading en-dash (
- **\ctx.positionals`**) is interpreted as an unordered-list bullet; Markdown renderers show an extra dash. Switching to a normal list item (* ctx.positionals`) or adding a blank line before it removes that artefact.Purely cosmetic, but worth tightening up the docs.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~190-~190: Possible missing comma found.
Context: ... error will occur. They cannot be truly optional like named flags. - Order Matters: ...(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~193-~193: Loose punctuation mark.
Context: ...s.destination). - **ctx.positionals`**: This array still exists and contains th...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/guide/essentials/declarative-configuration.md(5 hunks)src/renderer/usage.ts(7 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/guide/essentials/declarative-configuration.md
[uncategorized] ~190-~190: Possible missing comma found.
Context: ... error will occur. They cannot be truly optional like named flags. - Order Matters: ...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~193-~193: Loose punctuation mark.
Context: ...s.destination). - **ctx.positionals`**: This array still exists and contains th...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~218-~218: Loose punctuation mark.
Context: ...d context object (ctx) with: - args: The command arguments configuration (`A...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~219-~219: Loose punctuation mark.
Context: ...uration (ArgSchema object). - values: An object containing the resolved value...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~220-~220: Loose punctuation mark.
Context: ...s are stored as strings. - positionals: An array of strings containing the raw ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~221-~221: Loose punctuation mark.
Context: ...key>is generally recommended. -rest`: An array of strings containing argument...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~222-~222: Loose punctuation mark.
Context: ...ppear after the -- separator. - argv: The raw argument array passed to the `c...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~223-~223: Loose punctuation mark.
Context: ...passed to the cli function. - tokens: The raw tokens parsed by args-tokens....
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~224-~224: Loose punctuation mark.
Context: ...ens parsed by args-tokens. - omitted: A boolean indicating if the command was...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~225-~225: Loose punctuation mark.
Context: ...pecifying a subcommand name. - command: The resolved command definition object ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~226-~226: Loose punctuation mark.
Context: ...nition object itself. - commandOptions: The resolved command options passed to ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~227-~227: Loose punctuation mark.
Context: ...ommand options passed to cli. - name: The name of the currently executing c...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~228-~228: Loose punctuation mark.
Context: ...ntly executing_ command. - description: The description of the _currently execu...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~229-~229: Loose punctuation mark.
Context: ...e currently executing command. - env: The command environment settings (versi...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~230-~230: Loose punctuation mark.
Context: ...rsion, logger, renderers, etc.). - log: Logger function (defaults to `console.l...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 GitHub Check: Test on Node.js 20
src/renderer/usage.ts
[failure] 363-363: Unhandled error
TypeCheckError: Cannot find name 'getPositionalArgs'. Did you mean 'hasPositionalArgs'?
❯ src/renderer/usage.ts:363:23
[failure] 364-364: Unhandled error
TypeCheckError: Binding element 'name' implicitly has an 'any' type.
❯ src/renderer/usage.ts:364:55
[failure] 364-364: Unhandled error
TypeCheckError: Binding element '_' implicitly has an 'any' type.
❯ src/renderer/usage.ts:364:61
[failure] 367-367: Unhandled error
TypeCheckError: Binding element 'name' implicitly has an 'any' type.
❯ src/renderer/usage.ts:367:23
[failure] 367-367: Unhandled error
TypeCheckError: Binding element '_' implicitly has an 'any' type.
❯ src/renderer/usage.ts:367:29
[failure] 382-382: Unhandled error
TypeCheckError: Cannot find name 'getPositionalArgs'. Did you mean 'hasPositionalArgs'?
❯ src/renderer/usage.ts:382:10
[failure] 383-383: Unhandled error
TypeCheckError: Binding element 'name' implicitly has an 'any' type.
❯ src/renderer/usage.ts:383:16
[failure] 383-383: Unhandled error
TypeCheckError: Binding element '_' implicitly has an 'any' type.
❯ src/renderer/usage.ts:383:22
[failure] 383-383: Unhandled error
TypeCheckError: Parameter '__' implicitly has an 'any' type.
❯ src/renderer/usage.ts:383:26
[failure] 382-382: src/renderer.test.ts > renderUsage > positional arguments
ReferenceError: getPositionalArgs is not defined
❯ generatePositionalSymbols src/renderer/usage.ts:382:7
❯ renderUsageSection src/renderer/usage.ts:123:102
❯ renderUsage src/renderer/usage.ts:33:27
❯ src/renderer.test.ts:322:18
🪛 GitHub Actions: CI
src/renderer/usage.ts
[error] 363-363: TS2552: Cannot find name 'getPositionalArgs'. Did you mean 'hasPositionalArgs'?
[error] 364-364: TS7031: Binding element 'name' implicitly has an 'any' type.
[error] 364-364: TS7031: Binding element '_' implicitly has an 'any' type.
[error] 367-367: TS7031: Binding element 'name' implicitly has an 'any' type.
[error] 367-367: TS7031: Binding element '_' implicitly has an 'any' type.
[error] 382-382: TS2552: Cannot find name 'getPositionalArgs'. Did you mean 'hasPositionalArgs'?
[error] 383-383: TS7031: Binding element 'name' implicitly has an 'any' type.
[error] 383-383: TS7031: Binding element '_' implicitly has an 'any' type.
[error] 383-383: TS7006: Parameter '__' implicitly has an 'any' type.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
kazupon
changed the title
1 participant
Description
This PR resolves the values of positional arguments using the argument schema (
Args&ArgSchema).This allows values to be referenced by the names defined in the argument schema from
CommandContext.valuespassed torun.Linked Issues
resolve #108
Additional context
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores