feat(default): support default sub command#156
Conversation
fu050409
changed the title
|
We really need this! |
Support `default` option in command definition to specify a fallback sub command when no args are provided (resolves unjs#153). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughWhen a command has Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_Core as CLI Core
participant CmdResolver as Command Resolver
participant SubCmd as SubCommand Runner
User->>CLI_Core: invoke command (with or without subcommand token)
CLI_Core->>CmdResolver: parse opts, detect subCommands
CmdResolver->>CmdResolver: resolve `cmd.default` if present
alt `default` and top-level `run` present
CmdResolver-->>CLI_Core: throw E_DEFAULT_CONFLICT
CLI_Core-->>User: error
else `default` present
CmdResolver->>CmdResolver: validate default exists via subcommand lookup
alt default not found
CmdResolver-->>CLI_Core: throw E_UNKNOWN_COMMAND
CLI_Core-->>User: error
else default found
CmdResolver->>SubCmd: select subCommandName (explicit token or default)
alt explicit token present
CLI_Core->>SubCmd: execute with rawArgs sliced after token
else no explicit token (using default)
CLI_Core->>SubCmd: execute with original rawArgs
end
SubCmd-->>User: result
end
end
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/command.ts (1)
62-72:⚠️ Potential issue | 🟠 MajorDon't replay the parent's argv into the default child.
When there is no explicit subcommand token, Line 62 yields
-1, so Line 64 falls back todefaultSubCommandbut Line 71 still doesslice(0). A call likecli --config foowill hand--config footo the default child, so parent-only options get reparsed there instead of behaving likecli <default>. Either restrict the fallback to the true empty-argv case or strip the parent args before recursing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/command.ts` around lines 62 - 72, The code currently falls back to defaultSubCommand when findSubCommandIndex returns -1 but then passes opts.rawArgs.slice(subCommandArgIndex + 1) which becomes slice(0) and replay parent args into the child; fix by computing a childRawArgs variable: if subCommandArgIndex === -1 set childRawArgs = [] (strip parent args) otherwise set childRawArgs = opts.rawArgs.slice(subCommandArgIndex + 1), then call runCommand(subCommand, { rawArgs: childRawArgs }); update the block around findSubCommandIndex, subCommandArgIndex, defaultSubCommand and runCommand accordingly so the default subcommand doesn't reparse parent-only options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/command.ts`:
- Around line 37-44: resolveSubCommand() currently ignores cmd.default and only
treats an explicit bare token as a subcommand, causing inconsistency with
runCommand() which honors cmd.default; update resolveSubCommand() to thread the
command's default value (cmd.default) into its resolution logic so that when
rawArgs is empty or lacks an explicit subcommand it returns the resolved default
subcommand instead of the parent command. Specifically, inside
resolveSubCommand() (and the alternate branch referenced around lines 123-133),
consult resolveValue(cmd.default) the same way runCommand() does, validate
conflicts with cmd.run, and return the resolved default command token so both
entry points agree on the active command.
---
Outside diff comments:
In `@src/command.ts`:
- Around line 62-72: The code currently falls back to defaultSubCommand when
findSubCommandIndex returns -1 but then passes
opts.rawArgs.slice(subCommandArgIndex + 1) which becomes slice(0) and replay
parent args into the child; fix by computing a childRawArgs variable: if
subCommandArgIndex === -1 set childRawArgs = [] (strip parent args) otherwise
set childRawArgs = opts.rawArgs.slice(subCommandArgIndex + 1), then call
runCommand(subCommand, { rawArgs: childRawArgs }); update the block around
findSubCommandIndex, subCommandArgIndex, defaultSubCommand and runCommand
accordingly so the default subcommand doesn't reparse parent-only options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc725d13-3407-49ba-8906-3c2b5539caef
📒 Files selected for processing (3)
src/command.tssrc/types.tstest/main.test.ts
| // Resolve default sub command | ||
| const defaultSubCommand = await resolveValue(cmd.default); | ||
| if (defaultSubCommand && cmd.run) { | ||
| throw new CLIError( | ||
| `Command has a handler specified and a default sub command.`, | ||
| "E_DUPLICATE_COMMAND", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Thread default through resolveSubCommand() too.
runCommand() now honors cmd.default, but resolveSubCommand() still only resolves an explicit bare token and returns the parent command for rawArgs = []. That makes the two exported entry points disagree about which command is active for the same definition.
Also applies to: 123-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/command.ts` around lines 37 - 44, resolveSubCommand() currently ignores
cmd.default and only treats an explicit bare token as a subcommand, causing
inconsistency with runCommand() which honors cmd.default; update
resolveSubCommand() to thread the command's default value (cmd.default) into its
resolution logic so that when rawArgs is empty or lacks an explicit subcommand
it returns the resolved default subcommand instead of the parent command.
Specifically, inside resolveSubCommand() (and the alternate branch referenced
around lines 123-133), consult resolveValue(cmd.default) the same way
runCommand() does, validate conflicts with cmd.run, and return the resolved
default command token so both entry points agree on the active command.
Only resolve `cmd.default` when no explicit sub command arg is provided, avoiding unnecessary resolution in the common case. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/main.test.ts (1)
479-492: Strengthen the conflict test by asserting the CLI error code.The behavior contract includes
E_DUPLICATE_COMMAND; asserting code is less brittle than message text alone.Proposed test assertion update
- await expect(commandModule.runCommand(command, { rawArgs: [] })).rejects.toThrow( - /handler specified and a default sub command/, - ); + await expect(commandModule.runCommand(command, { rawArgs: [] })).rejects.toMatchObject({ + name: "CLIError", + code: "E_DUPLICATE_COMMAND", + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main.test.ts` around lines 479 - 492, Update the test that expects a rejection when both a default and run handler are specified to also assert the CLI error code; specifically, when calling commandModule.runCommand with the command created by defineCommand (the test named "throws when both default and run are specified"), capture the thrown error and assert that error.code === "E_DUPLICATE_COMMAND" in addition to the existing message assertion to make the test less brittle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/main.test.ts`:
- Around line 479-492: Update the test that expects a rejection when both a
default and run handler are specified to also assert the CLI error code;
specifically, when calling commandModule.runCommand with the command created by
defineCommand (the test named "throws when both default and run are specified"),
capture the thrown error and assert that error.code === "E_DUPLICATE_COMMAND" in
addition to the existing message assertion to make the test less brittle.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #156 +/- ##
=======================================
Coverage ? 97.65%
=======================================
Files ? 8
Lines ? 384
Branches ? 134
=======================================
Hits ? 375
Misses ? 8
Partials ? 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Validate `default + run` conflict eagerly, check that default references an existing sub command, pass full rawArgs when using default, and fix the type definition.
avoid eagerly resolving `cmd.default` and loading the default subcommand when an explicit sub command is provided
3 participants
Support default sub command
Resolved: #153
Summary by CodeRabbit