fix(parser): handle short alias with = syntax (-n=value)#240
Open
guoyangzhen wants to merge 1 commit intounjs:mainfrom
Open
fix(parser): handle short alias with = syntax (-n=value)#240guoyangzhen wants to merge 1 commit intounjs:mainfrom
guoyangzhen wants to merge 1 commit intounjs:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughParser narrows registered options to long/multi-character names (booleans, strings, defaults, multi-char aliases), collects single-character short aliases separately, and adds type-aware coercion: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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
🤖 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 72-76: longOptions is being seeded with Object.keys(defaults)
which can include single-character short aliases (e.g., "n") and erroneously
promote them to full options; modify the longOptions initialization to only
include default keys that are longer than one character (e.g., filter
Object.keys(defaults).filter(k => k.length > 1)) so booleans and strings stay
unchanged and short aliases remain aliases; update the Set construction that
references longOptions accordingly (the line creating longOptions that spreads
booleans, strings, and Object.keys(defaults)).
Member
|
Can you please add tests? |
Author
|
Done! Changed
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/parser.test.ts (1)
59-74: Exercise both aliases (or rename the test) for clarity.The case defines two short aliases but only validates
-o. Either add a second assertion for-Oor narrow the test title.✅ Suggested test extension
it("handles -<arg>=<value> with multiple short aliases", () => { const result = parseRawArgs(["-o=output.txt"], { string: ["output", "out"], alias: { o: ["output"], O: ["out"], }, }); expect(result).toEqual({ _: [], o: "output.txt", output: "output.txt", }); + + const resultUpper = parseRawArgs(["-O=out.txt"], { + string: ["output", "out"], + alias: { + o: ["output"], + O: ["out"], + }, + }); + + expect(resultUpper).toEqual({ + _: [], + O: "out.txt", + out: "out.txt", + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/parser.test.ts` around lines 59 - 74, The test "handles -<arg>=<value> with multiple short aliases" in parser.test.ts defines two short aliases (o and O) for parseRawArgs but only exercises -o; update the test to either also call parseRawArgs(["-O=output.txt"], {...}) and add corresponding expectations for O and out/output, or rename the test title to reflect that it only checks the -o alias; locate the test case and modify the parseRawArgs invocation and expect block (or the it() description) accordingly so both aliases are exercised or the title accurately describes the single-alias check.
🤖 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/parser.test.ts`:
- Around line 59-74: The test "handles -<arg>=<value> with multiple short
aliases" in parser.test.ts defines two short aliases (o and O) for parseRawArgs
but only exercises -o; update the test to either also call
parseRawArgs(["-O=output.txt"], {...}) and add corresponding expectations for O
and out/output, or rename the test title to reflect that it only checks the -o
alias; locate the test case and modify the parseRawArgs invocation and expect
block (or the it() description) accordingly so both aliases are exercised or the
title accurately describes the single-alias check.
guoyangzhen
pushed a commit
to guoyangzhen/citty
that referenced
this pull request
Mar 17, 2026
Follow-up to unjs#240. Filter single-char keys from defaults when building longOptions to prevent them from being re-registered as full options. Defaults for short aliases are still applied to their long-form counterparts. Addresses CodeRabbit review feedback.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/_parser.ts (1)
81-116:⚠️ Potential issue | 🟠 MajorSingle-character defaults for real options are being dropped.
Object.keys(defaults).filter((k) => k.length > 1)removes defaults for legitimate one-character options (e.g.default: { x: "1" }) even whenxis not a short alias. The remap block only restores keys found inaliasToMain, so non-aliased one-char defaults are lost.Proposed fix
const longOptions = new Set<string>([ ...booleans, ...strings, - ...Object.keys(defaults).filter((k) => k.length > 1), + ...Object.keys(defaults).filter((k) => !shortAliasNames.has(k)), ]); @@ - for (const key of Object.keys(defaults)) { - if (key.length === 1) { + for (const key of Object.keys(defaults)) { + if (shortAliasNames.has(key)) { const longForm = aliasToMain.get(key); if (longForm && !longOptions.has(longForm)) { options[longForm] = { type: getType(longForm), default: defaults[key],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/_parser.ts` around lines 81 - 116, The code drops default values for legitimate single-character options because longOptions is initialized using Object.keys(defaults).filter(k => k.length > 1) and the restore only handles keys present in aliasToMain; fix by ensuring single-char defaults that are not aliases are registered: when iterating Object.keys(defaults) if key.length === 1 and aliasToMain.get(key) is falsy, create an options entry for that single-char key (use getType(key) and defaults[key]) and (optionally) add it to longOptions or another tracking set so its default isn't lost; keep the existing alias-handling branch that maps single-char aliases to their longForm when aliasToMain has a value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/_parser.ts`:
- Around line 81-116: The code drops default values for legitimate
single-character options because longOptions is initialized using
Object.keys(defaults).filter(k => k.length > 1) and the restore only handles
keys present in aliasToMain; fix by ensuring single-char defaults that are not
aliases are registered: when iterating Object.keys(defaults) if key.length === 1
and aliasToMain.get(key) is falsy, create an options entry for that single-char
key (use getType(key) and defaults[key]) and (optionally) add it to longOptions
or another tracking set so its default isn't lost; keep the existing
alias-handling branch that maps single-char aliases to their longForm when
aliasToMain has a value.
Author
|
Updated to latest upstream code. The parser fix filters single-char aliases from |
…--key=val (unjs#237) - Filter single-char alias names from allOptions so they are only registered via node:util parseArgs property - Add preprocessing to expand -<char>=<value> to --<main>=<value> since node:util parseArgs does not handle = syntax for short aliases - Fix from 653b15a was already upstreamed; this is the remaining piece - 17→19 tests (added multiple short aliases + unknown flag edge cases) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Author
|
@pi0 Done! I rebased onto the latest main — the
19/19 tests pass. Also squashed the previous messy commit history into a clean single commit. |
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.
Resolves #237
Problem
-n=Johnwith short aliasnfornamereturns{ n: "=John", name: "=John" }instead of{ n: "John", name: "John" }.Root cause:
node:util parseArgsdoes not handle-<char>=<value>syntax for short aliases. It treats-n=Johnas-nwith value=John.Fix
Two-part fix in
src/_parser.ts:Filter single-char aliases from
allOptions— aliases likenshould only be registered via theshortproperty of their target option, not as standalone options. OtherwiseparseArgsgets confused whennappears as both a full option and a short alias.Preprocess
-<char>=<value>syntax — before passing args toparseArgs, expand-n=Johnto--name=Johnwhennis a known alias forname. This bypasses theparseArgslimitation entirely.Tests
it.failstest now passes → changed toit=syntax=passes through without crashRebased
Rebased on latest
mainto resolve merge conflicts. The parser changes from upstream (alias coercion, type handling) are preserved — this PR now only adds the=syntax handling.