Deduplicate JSON string escaping helpers#305
Merged
Merged
Conversation
- Add shared JSON escape/quote utilities - Replace local escape implementations in runners, profiler, source maps, and JSON code - Keep JSON5/TOML check output consistent
Contributor
📝 WalkthroughWalkthroughCentralizes JSON string escaping by adding Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
units/Goccia.JSON.Utils.pas (1)
26-60: Add direct coverage for the shared escaper.This is now a single point of failure for several JSON emitters. A small focused matrix around
",\,/,#0,#31, and multibyte UTF-8 would make regressions much easier to catch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.JSON.Utils.pas` around lines 26 - 60, Add direct unit tests that exercise EscapeJSONString and QuoteJSONString: create focused tests that pass strings containing the characters '"' , '\' , '/' , `#0` (Chr(0)), `#31` (Chr(31)) and a multibyte UTF-8 sequence (e.g., non-ASCII runes) to verify proper escaping and quoting. For each case assert EscapeJSONString returns the expected escaped body and QuoteJSONString wraps that result in double quotes; include at least one test combining multiple special chars to validate buffering logic and one test ensuring non-control multibyte characters are preserved (not escaped as \u unless below codepoint 32). Use the function names EscapeJSONString and QuoteJSONString to locate coverage gaps and add tests to the same test unit as other JSON utils.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@units/Goccia.JSON.Utils.pas`:
- Around line 26-60: Add direct unit tests that exercise EscapeJSONString and
QuoteJSONString: create focused tests that pass strings containing the
characters '"' , '\' , '/' , `#0` (Chr(0)), `#31` (Chr(31)) and a multibyte UTF-8
sequence (e.g., non-ASCII runes) to verify proper escaping and quoting. For each
case assert EscapeJSONString returns the expected escaped body and
QuoteJSONString wraps that result in double quotes; include at least one test
combining multiple special chars to validate buffering logic and one test
ensuring non-control multibyte characters are preserved (not escaped as \u
unless below codepoint 32). Use the function names EscapeJSONString and
QuoteJSONString to locate coverage gaps and add tests to the same test unit as
other JSON utils.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d26699a-9af0-4f3e-9405-d0c12c5e04ce
📒 Files selected for processing (8)
TestRunner.dprscripts/GocciaJSON5Check.dprscripts/GocciaTOMLCheck.dprunits/Goccia.JSON.Utils.pasunits/Goccia.JSON.pasunits/Goccia.Profiler.Report.pasunits/Goccia.ScriptLoader.JSON.pasunits/Goccia.SourceMap.pas
Contributor
Suite Timing
Measured on ubuntu-latest x64. |
Contributor
Benchmark Results364 benchmarks Interpreted: 🟢 19 improved · 🔴 230 regressed · 115 unchanged · avg -2.4% arraybuffer.js — Interp: 🔴 5, 9 unch. · avg -0.8% · Bytecode: 🟢 4, 🔴 1, 9 unch. · avg +0.6%
arrays.js — Interp: 🟢 2, 🔴 12, 5 unch. · avg -1.7% · Bytecode: 🟢 6, 🔴 5, 8 unch. · avg +0.6%
async-await.js — Interp: 🔴 4, 2 unch. · avg -2.1% · Bytecode: 🟢 1, 🔴 1, 4 unch. · avg -0.1%
base64.js — Interp: 🟢 1, 🔴 6, 3 unch. · avg -2.1% · Bytecode: 🟢 2, 🔴 7, 1 unch. · avg -0.9%
classes.js — Interp: 🟢 1, 🔴 10, 20 unch. · avg -0.9% · Bytecode: 🟢 10, 🔴 1, 20 unch. · avg +2.6%
closures.js — Interp: 🔴 9, 2 unch. · avg -2.2% · Bytecode: 🟢 4, 🔴 1, 6 unch. · avg +1.7%
collections.js — Interp: 🟢 1, 🔴 8, 3 unch. · avg -1.9% · Bytecode: 🟢 5, 🔴 3, 4 unch. · avg +1.2%
destructuring.js — Interp: 🟢 3, 🔴 7, 12 unch. · avg -1.2% · Bytecode: 🟢 14, 🔴 1, 7 unch. · avg +3.1%
fibonacci.js — Interp: 🔴 7, 1 unch. · avg -2.3% · Bytecode: 🟢 2, 🔴 2, 4 unch. · avg +0.8%
float16array.js — Interp: 🟢 1, 🔴 25, 6 unch. · avg -3.4% · Bytecode: 🟢 6, 🔴 16, 10 unch. · avg -8.4%
for-of.js — Interp: 🔴 1, 6 unch. · avg -0.7% · Bytecode: 🟢 4, 3 unch. · avg +2.0%
helpers/bench-module.js — Interp: 0 · Bytecode: 0
iterators.js — Interp: 🔴 28, 14 unch. · avg -2.8% · Bytecode: 🟢 25, 🔴 1, 16 unch. · avg +2.2%
json.js — Interp: 🟢 6, 🔴 7, 7 unch. · avg -0.4% · Bytecode: 🟢 11, 9 unch. · avg +2.9%
jsx.jsx — Interp: 🟢 2, 🔴 13, 6 unch. · avg -1.9% · Bytecode: 🟢 3, 🔴 1, 17 unch. · avg +0.6%
modules.js — Interp: 🟢 1, 🔴 7, 1 unch. · avg -4.5% · Bytecode: 🟢 1, 🔴 7, 1 unch. · avg -1.1%
numbers.js — Interp: 🔴 10, 1 unch. · avg -3.2% · Bytecode: 🟢 5, 🔴 1, 5 unch. · avg +1.7%
objects.js — Interp: 🔴 6, 1 unch. · avg -3.6% · Bytecode: 🟢 3, 🔴 1, 3 unch. · avg +0.3%
promises.js — Interp: 🔴 11, 1 unch. · avg -3.9% · Bytecode: 🟢 1, 🔴 1, 10 unch. · avg -0.1%
regexp.js — Interp: 🔴 4, 7 unch. · avg -1.0% · Bytecode: 🟢 3, 8 unch. · avg +0.6%
strings.js — Interp: 🔴 16, 3 unch. · avg -3.3% · Bytecode: 🟢 6, 🔴 5, 8 unch. · avg -0.1%
typed-arrays.js — Interp: 🔴 20, 2 unch. · avg -4.0% · Bytecode: 🟢 7, 🔴 4, 11 unch. · avg +0.7%
uint8array-encoding.js — Interp: 🟢 1, 🔴 14, 3 unch. · avg -4.7% · Bytecode: 🟢 7, 🔴 4, 7 unch. · avg +1.4%
Measured on ubuntu-latest x64. Benchmark ranges compare cached main-branch min/max ops/sec with the PR run; overlapping ranges are treated as unchanged noise. Percentage deltas are secondary context. |
- Cover escaping for quotes, backslashes, solidus, and control chars - Verify multibyte UTF-8 passthrough and quoted output wrapping
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
units/Goccia.JSON.Utils.Test.pas (1)
91-101: Align multibyte UTF-8 comment with actual assertions.Line 93 mentions U+1F600 coverage, but the test body only checks U+00E9 cases. Either add an emoji assertion or trim the comment.
Suggested update
procedure TJSONUtilsTests.TestPreservesMultibyteUTF8; var EAcute: string; + GrinningFace: string; begin EAcute := #$C3#$A9; // UTF-8 for U+00E9 (e with acute accent) + GrinningFace := #$F0#$9F#$98#$80; // UTF-8 for U+1F600 (grinning face) Expect<string>(EscapeJSONString(EAcute)).ToBe(EAcute); Expect<string>(EscapeJSONString('caf' + EAcute)).ToBe('caf' + EAcute); + Expect<string>(EscapeJSONString(GrinningFace)).ToBe(GrinningFace); end;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@units/Goccia.JSON.Utils.Test.pas` around lines 91 - 101, The comment for TJSONUtilsTests.TestPreservesMultibyteUTF8 mentions U+1F600 but the test only asserts U+00E9; update the test to match the comment or simplify the comment: either add a four-byte UTF‑8 assertion by creating a GrinningEmoji variable (e.g., Grinning := #$F0#$9F#$98#$80) and assert Expect<string>(EscapeJSONString(Grinning)).ToBe(Grinning) and Expect<string>(EscapeJSONString('face' + Grinning)).ToBe('face' + Grinning), referencing EscapeJSONString and EAcute as examples, or remove the U+1F600 mention from the comment so it only documents the U+00E9 checks in TestPreservesMultibyteUTF8.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@units/Goccia.JSON.Utils.Test.pas`:
- Around line 91-101: The comment for TJSONUtilsTests.TestPreservesMultibyteUTF8
mentions U+1F600 but the test only asserts U+00E9; update the test to match the
comment or simplify the comment: either add a four-byte UTF‑8 assertion by
creating a GrinningEmoji variable (e.g., Grinning := #$F0#$9F#$98#$80) and
assert Expect<string>(EscapeJSONString(Grinning)).ToBe(Grinning) and
Expect<string>(EscapeJSONString('face' + Grinning)).ToBe('face' + Grinning),
referencing EscapeJSONString and EAcute as examples, or remove the U+1F600
mention from the comment so it only documents the U+00E9 checks in
TestPreservesMultibyteUTF8.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 101a8537-8e01-4b6a-9260-132c4518892e
📒 Files selected for processing (1)
units/Goccia.JSON.Utils.Test.pas
- Verify EscapeJSONString preserves 4-byte UTF-8 sequences - Cover plain emoji and mixed ASCII/emoji input
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
1 participant
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.
Summary
Goccia.JSON.Utilsto centralize JSON string escaping and quoting helpers.EscapeJSONString/QuoteJSONStringimplementations in the test runner, JSON5/TOML checks, script loader JSON, profiler report, source map, and core JSON code.Fixes #303
Testing
Goccia.JSON.Utilsand no longer define local escaping helpers.\uXXXXfallback for other C0 controls.