fix: skip parent run() when subcommand executes, support = in alias values - #265
fix: skip parent run() when subcommand executes, support = in alias values#265marceli1404 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe parser now supports ChangesArgument parsing
Subcommand execution
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/main.test.ts (1)
146-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for default subcommand dispatch.
This test verifies the explicit
["foo"]path, butsrc/command.tsalso changed the default-subcommand path. Add a case withdefault: "foo"and no positional subcommand to confirm the parentrun()remains skipped and the default child runs once.Suggested test coverage
+ it("does not run parent command's run() when the default subcommand is executed", async () => { + const parentRunMock = vi.fn(); + const subRunMock = vi.fn(); + + const command = defineCommand({ + default: "foo", + subCommands: { + foo: { run: async () => subRunMock() }, + }, + run: async () => parentRunMock(), + }); + + await runMain(command, { rawArgs: [] }); + + expect(parentRunMock).not.toHaveBeenCalled(); + expect(subRunMock).toHaveBeenCalledOnce(); + });🤖 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 `@test/main.test.ts` around lines 146 - 167, Extend the test around defineCommand and runMain to cover default subcommand dispatch by configuring subCommands.foo as the default, invoking runMain without a positional subcommand, and asserting parentRunMock is not called while subRunMock is called once. Keep the existing explicit ["foo"] coverage unchanged.
🤖 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 `@src/_parser.ts`:
- Around line 127-132: Restrict the single-dash equals-sign preprocessing in the
argument-processing flow to tokens whose alias portion is exactly one character
and is present in the configured aliases. Preserve the original argument for
unrecognized or multi-character forms, including hyphen-prefixed values and
boolean forms such as -v=false, while retaining splitting for recognized
-x=value aliases.
---
Nitpick comments:
In `@test/main.test.ts`:
- Around line 146-167: Extend the test around defineCommand and runMain to cover
default subcommand dispatch by configuring subCommands.foo as the default,
invoking runMain without a positional subcommand, and asserting parentRunMock is
not called while subRunMock is called once. Keep the existing explicit ["foo"]
coverage unchanged.
🪄 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 Plus
Run ID: 04f32a58-7d15-4803-9dbb-0a965a97d1c7
📒 Files selected for processing (4)
src/_parser.tssrc/command.tstest/main.test.tstest/parser.test.ts
| // Handle -x=value (single-dash alias with = separator) | ||
| if (arg.startsWith("-") && !arg.startsWith("--") && arg.includes("=")) { | ||
| const eqIndex = arg.indexOf("="); | ||
| processedArgs.push(arg.slice(0, eqIndex), arg.slice(eqIndex + 1)); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict preprocessing to recognized one-character aliases.
This condition rewrites every single-dash token containing =, not only -x=value aliases. That regresses valid hyphen-prefixed string values such as ["--name", "-test=value"], which are split before parseArgs can consume them as one value. It can also misparse boolean forms such as -v=false.
Gate this transformation on a one-character, configured alias; otherwise preserve the original argument.
Proposed fix
+ const eqIndex = arg.indexOf("=");
- if (arg.startsWith("-") && !arg.startsWith("--") && arg.includes("=")) {
- const eqIndex = arg.indexOf("=");
+ const alias = eqIndex > 0 ? arg.slice(1, eqIndex) : "";
+ if (
+ arg.startsWith("-") &&
+ !arg.startsWith("--") &&
+ eqIndex === 2 &&
+ aliasToMain.has(alias)
+ ) {
processedArgs.push(arg.slice(0, eqIndex), arg.slice(eqIndex + 1));
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle -x=value (single-dash alias with = separator) | |
| if (arg.startsWith("-") && !arg.startsWith("--") && arg.includes("=")) { | |
| const eqIndex = arg.indexOf("="); | |
| processedArgs.push(arg.slice(0, eqIndex), arg.slice(eqIndex + 1)); | |
| continue; | |
| } | |
| // Handle -x=value (single-dash alias with = separator) | |
| const eqIndex = arg.indexOf("="); | |
| const alias = eqIndex > 0 ? arg.slice(1, eqIndex) : ""; | |
| if ( | |
| arg.startsWith("-") && | |
| !arg.startsWith("--") && | |
| eqIndex === 2 && | |
| aliasToMain.has(alias) | |
| ) { | |
| processedArgs.push(arg.slice(0, eqIndex), arg.slice(eqIndex + 1)); | |
| continue; | |
| } |
🤖 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 `@src/_parser.ts` around lines 127 - 132, Restrict the single-dash equals-sign
preprocessing in the argument-processing flow to tokens whose alias portion is
exactly one character and is present in the configured aliases. Preserve the
original argument for unrecognized or multi-character forms, including
hyphen-prefixed values and boolean forms such as -v=false, while retaining
splitting for recognized -x=value aliases.
Fixes
#253 - Parent command's run() executes when subcommand runs
When a subcommand is explicitly provided and executed, the parent command's
run()function was still being called. This caused both the subcommand and parent to execute theirrun()logic.Fix: Added a
subCommandExecutedflag that is set totruewhen a subcommand is found and executed. The parent'srun()is now gated behind!subCommandExecuted.#237 - Alias values with
=separator (e.g.-n=John)The native Node.js
parseArgsincludes the=in the value when a single-dash alias uses=syntax (e.g.,-n=Johnis parsed asn: "=John").Fix: Added preprocessing in
parseRawArgsthat splits single-dash args on the first=into separate tokens ([-n, John]) before passing them to Node.jsparseArgs.Summary by CodeRabbit
New Features
-x=value, treating them consistently with option aliases.Bug Fixes
Tests