fix: coerce parsed arg values to match declared types#239
Conversation
📝 WalkthroughWalkthroughThe argument parser now applies type-aware value coercion during parsing. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 Tip Migrating from UI to YAML configuration.Use the |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/args.test.ts (1)
128-132: Add one alias-path assertion for empty-string coercion.You’re testing
--nightlydirectly; adding an alias case (e.g.-n) would directly exercise the alias-aware path used in parser coercion.Proposed test addition
it("should return empty string for string arg without value", () => { const parsed = parseArgs(["--nightly"], { nightly: { type: "string" } }); expect(parsed.nightly).toBe(""); expect(typeof parsed.nightly).toBe("string"); }); + + it("should return empty string for aliased string arg without value", () => { + const parsed = parseArgs(["-n"], { nightly: { type: "string", alias: "n" } }); + expect(parsed.nightly).toBe(""); + expect(parsed.n).toBe(""); + expect(typeof parsed.nightly).toBe("string"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/args.test.ts` around lines 128 - 132, Add an additional assertion in the "should return empty string for string arg without value" test to exercise the alias-aware path: call parseArgs with the alias form (e.g., ["-n"]) using the same options ({ nightly: { type: "string", alias: "n" } } or ensure alias mapping is present) and assert that parsed.nightly === "" and typeof parsed.nightly === "string" so the parser's alias coercion branch (parseArgs) is covered.
🤖 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/_parser.ts`:
- Around line 154-155: The boolean string coercion currently only checks value
!== "false" which misclassifies inputs like "False" or " false "; update the
branch in src/_parser.ts where type === "boolean" && typeof value === "string"
to normalize the string first (e.g., value.trim().toLowerCase()) and then
compare to "false" before setting coerced; ensure you reference the variables
type, value, and coerced so the normalized comparison is used when assigning
coerced.
---
Nitpick comments:
In `@test/args.test.ts`:
- Around line 128-132: Add an additional assertion in the "should return empty
string for string arg without value" test to exercise the alias-aware path: call
parseArgs with the alias form (e.g., ["-n"]) using the same options ({ nightly:
{ type: "string", alias: "n" } } or ensure alias mapping is present) and assert
that parsed.nightly === "" and typeof parsed.nightly === "string" so the
parser's alias coercion branch (parseArgs) is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9aa0cebf-84e5-460b-81d3-fe8df3fbcd8d
📒 Files selected for processing (3)
src/_parser.tstest/args.test.tstest/parser.test.ts
| if (type === "boolean" && typeof value === "string") { | ||
| coerced = value !== "false"; |
There was a problem hiding this comment.
Normalize boolean string values before false-check.
Line 155 currently treats only exact "false" as false. Inputs like --flag=False or --flag=" false " will be coerced to true.
Proposed fix
- if (type === "boolean" && typeof value === "string") {
- coerced = value !== "false";
+ if (type === "boolean" && typeof value === "string") {
+ const normalized = value.trim().toLowerCase();
+ coerced = normalized !== "false";📝 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.
| if (type === "boolean" && typeof value === "string") { | |
| coerced = value !== "false"; | |
| if (type === "boolean" && typeof value === "string") { | |
| const normalized = value.trim().toLowerCase(); | |
| coerced = normalized !== "false"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/_parser.ts` around lines 154 - 155, The boolean string coercion currently
only checks value !== "false" which misclassifies inputs like "False" or " false
"; update the branch in src/_parser.ts where type === "boolean" && typeof value
=== "string" to normalize the string first (e.g., value.trim().toLowerCase())
and then compare to "false" before setting coerced; ensure you reference the
variables type, value, and coerced so the normalized comparison is used when
assigning coerced.
node:util.parseArgswithstrict:falsecan return wrong types:--flag=true/--flag=falsereturn strings'true'/'false'--flag) return booleantrueinstead of''Summary by CodeRabbit
New Features
Tests