fix: escape normalized keys in no-unnormalized-keys autofix - #283
fix: escape normalized keys in no-unnormalized-keys autofix#283electrohyun wants to merge 3 commits into
no-unnormalized-keys autofix#283Conversation
|
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 (2)
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 Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Normalized string-key autofixes now preserve valid JSON-family syntax when normalization produces quotes or backslashes. Coverage includes the affected formats and normalization forms, with no current merge-blocking risk identified. Suggested reviewers: 🚥 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 |
| }, | ||
| { | ||
| 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}`;
}4 participants
Prerequisites checklist
AI acknowledgment
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
Refactor
Tests