fix: escape normalized keys in no-unnormalized-keys autofix - #283
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe ChangesNormalized key autofix
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
| }, | ||
| { | ||
| code: `{"a＂b＼c＇＂＼": 1}`, | ||
| output: `{"a\\"b\\\\c'\\"\\\\": 1}`, |
There was a problem hiding this comment.
Should we use String.raw for readability here?
There was a problem hiding this comment.
I agree that using String.raw for every output property would be more readable.
DMartens
left a comment
DMartens
left a comment
There was a problem hiding this comment.
Thank you for the PR.
The logic looks good to me, just one suggested refactoring and additional test cases.
| : name.range, | ||
| normalizedKey, | ||
| ); | ||
| if (name.type === "String") { |
There was a problem hiding this comment.
We could simplify this as only the "key" is changed:
const fixedKey = name.type === "String" ? escapeKey(...) : normalizedKey;
return fixer.replaceText(name, fixedKey);and extract the quote + replaceAll logic into a function (which now adds the same quotes around the key as the whole key is replaced).
As you mentioned it is hard to come up with a better name as normalizedKey is in scope.
| ], | ||
| }, | ||
| { | ||
| code: `{"a＂b＼c＇＂＼": 1}`, |
There was a problem hiding this comment.
Can you please add separate test cases for one options.form for the quotes and escaping logic (e.g. { "a'b": 1 } showing that the not used quote is not escaped).
You can keep the "combined" test cases.
|
@DMartens Applied. I think |
| ], | ||
| }, | ||
| { | ||
| code: `{"a＂b": 1}`, |
There was a problem hiding this comment.
| code: `{"a＂b": 1}`, | |
| code: `{"a\uff02b": 1}`, |
Disclosure: I'm a participant of open source contribution program OSSCA: confirmed.
If using U+FF02 is the intention of this test, could we use the Unicode escape sequence instead? In some code editors or IDEs, it may look similar to U+0022, which could confuse people when reading the test cases.
It would also be helpful to update the other test cases affected by this.
| ); | ||
| ? escapeKey( | ||
| normalizedKey, | ||
| context.sourceCode.getText(name), |
There was a problem hiding this comment.
| context.sourceCode.getText(name), | |
| sourceCode.getText(name), |
+ const { sourceCode } = context;
const [{ form }] = context.options;Small suggestion: I think we could destructure sourceCode at the top to avoid the minor overhead of accessing it each time, since there’s one more occurrence of context.sourceCode.
| ); | ||
| ? escapeKey( | ||
| normalizedKey, | ||
| context.sourceCode.getText(name), |
There was a problem hiding this comment.
I am not sure we need to call the getText method here. We can simply access the quote information as follows:
const fixedKey =
name.type === "String"
? escapeKey(
normalizedKey,
context.sourceCode.text[
name.range[0]
],
)
: normalizedKey;The helper function could then be simplified as follows:
/**
* Escapes a normalized string key and wraps it in its original quotes.
* @param {string} normalizedKey The normalized key to escape.
* @param {string} quote The quote character used in the original key.
* @returns {string} The escaped and quoted key.
*/
function escapeKey(normalizedKey, quote) {
const escapedKey = normalizedKey
.replaceAll("\\", "\\\\")
.replaceAll(quote, `\\${quote}`);
return `${quote}${escapedKey}${quote}`;
}
|
@lumirlumir Applied. Thank you for suggesting this structure! |
DMartens
left a comment
DMartens
left a comment
There was a problem hiding this comment.
Changes LGTM, thanks. Leaving open for lumir.
| const fixedKey = | ||
| name.type === "String" | ||
| ? [name.range[0] + 1, name.range[1] - 1] | ||
| : name.range, | ||
| normalizedKey, | ||
| ); | ||
| ? escapeKey( | ||
| normalizedKey, | ||
| sourceCode.text[name.range[0]], | ||
| ) | ||
| : normalizedKey; | ||
|
|
||
| return fixer.replaceText(name, fixedKey); |
There was a problem hiding this comment.
The behavior would be the same, but rather than reconstructing the key by concatenating strings based on the quote information, I think we can simply escape the normalized key here.
To me, the original type === "String" ? [range[0] + 1, range[1] - 1] : range more clearly represents the original intent and makes it clear exactly where this fix applies.
/**
* Escapes a normalized string key for use inside its original quotes.
* @param {string} normalizedKey The normalized key to escape.
* @param {string} quote The quote character used in the original key.
* @returns {string} The escaped and quoted key.
*/
function escapeKey(normalizedKey, quote) {
return normalizedKey
.replaceAll("\\", "\\\\")
.replaceAll(quote, `\\${quote}`);
}const { loc, range, type } = node.name; return fixer.replaceTextRange(
type === "String"
? [range[0] + 1, range[1] - 1]
: range,
type === "String"
? escapeKey(
normalizedKey,
sourceCode.text[range[0]],
)
: normalizedKey,
);
|
Destructuring I’ll keep the original approach while retaining the helper extraction suggested in the earlier review. Thanks for another review! |
DMartens
left a comment
DMartens
left a comment
There was a problem hiding this comment.
Changes LGTM, thanks. Leaving open for lumir.
4 participants
Prerequisites checklist
AI acknowledgment
I used AI to check the English grammar in review responses I wrote myself. The technical decisions in those responses were my own.
What is the purpose of this pull request?
Fix
no-unnormalized-keysautofix producing invalid JSON when normalization introduces quotes or\.What changes did you make? (Give an overview)
Escape
\and matching quotes in normalized keys.Add regression tests for
NFKCandNFKDnormalization across JSON, JSONC, and JSON5 (single quote cases are included only in JSON5).Related Issues
fixes #282
Is there anything you'd like reviewers to focus on?
At first, I wrote code with a
letvariable, but it ended up using that variable’s name instead ofnormalizedKey, and I think this is less consistent with the rule nameno-unnormalized-keys.The current approach does the same by using a different name,
escapedKey, but only for string keys andnormalizedKeyis still used for identifier keys, so I went with this structure.If there’s a better way to structure the fixer, please let me know.
Disclosure: I'm a participant of open source contribution program OSSCA
Summary by CodeRabbit
Bug Fixes
Tests