feat(default): support default sub command#156
Open
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>
📝 WalkthroughWalkthroughWhen a command has subCommands and a 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 (no subcommand token)
CLI_Core->>CmdResolver: parse args, detect no subcommand
CmdResolver->>CmdResolver: check `cmd.default`
alt default present and main has no run
CmdResolver->>SubCmd: resolve default subcommand name
CLI_Core->>SubCmd: execute subcommand
SubCmd-->>User: return result
else default present and main has run
CmdResolver-->>User: throw E_DUPLICATE_COMMAND
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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
src/command.ts
Outdated
Comment on lines
+37
to
+44
| // 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Support default sub command
Resolved: #153
Summary by CodeRabbit