fix: read --file inputs with utf-8-sig to strip UTF-8 BOM - #4
Conversation
On Windows, PowerShell's Out-File -Encoding utf8 writes a UTF-8 BOM (\xef\xbb\xbf) by default. Python's json.load treats the BOM as invalid content, raising: JSONDecodeError: Expecting value: line 1 column 1 (char 0) Switch --file open() calls from encoding='utf-8' (or implicit default) to encoding='utf-8-sig', which auto-strips a leading BOM and is fully backward-compatible with non-BOM files. Affects 8 sites in cli_charts/cmd/_helpers.py (animate, record, code, mermaid, diagram, sequence, and friends) and 1 site in cli_charts/dashboard.py. Adds tests/test_bom_compat.py covering all 5 chart types (bar, sparkline, line, pie, gauge), dashboard, the raw open() behavior, and a negative test demonstrating the original bug.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds UTF-8 BOM (Byte Order Mark) handling to file inputs across the CLI by updating ChangesUTF-8 BOM Compatibility
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 docstrings
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 |
|
🚨 Change risk: 7.5/10 (high)
🔥 Hotspots touched (2)
🔗 Hidden coupling (2 files)
💀 Dead code (4 findings)
1 more
👀 Suggested reviewers @2233admin @Curry 📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-06-05 04:46 UTC |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates file reading operations in cli_charts/cmd/_helpers.py and cli_charts/dashboard.py to use utf-8-sig encoding, ensuring that UTF-8 BOM prefixes (commonly written by PowerShell on Windows) are automatically stripped and parsed correctly. It also adds a comprehensive test suite in tests/test_bom_compat.py to verify BOM compatibility. The review feedback suggests strengthening these integration tests by asserting that the subprocess command execution returns a successful exit code (rc == 0), which prevents false positives if the command fails for other reasons.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| assert "ERROR:json" not in err, ( | ||
| f"BOM file failed JSON load for {chart_type}\\nstderr: {err}" | ||
| ) |
There was a problem hiding this comment.
The test currently only asserts that "ERROR:json" is not in stderr. However, if the command fails for other reasons (such as missing dependencies, environment issues, or other exceptions), the test might silently pass because "ERROR:json" is not present in the output. Asserting that the return code rc is 0 ensures that the command executed successfully and the test is robust against false positives.
| assert "ERROR:json" not in err, ( | |
| f"BOM file failed JSON load for {chart_type}\\nstderr: {err}" | |
| ) | |
| assert rc == 0, f"Command failed with exit code {rc}\nstdout: {out}\nstderr: {err}" | |
| assert "ERROR:json" not in err, ( | |
| f"BOM file failed JSON load for {chart_type}\nstderr: {err}" | |
| ) |
| with open(path, "w", encoding="utf-8") as f: | ||
| json.dump(payload, f) | ||
| rc, out, err = _run("bar", path) | ||
| assert "ERROR:json" not in err, err |
There was a problem hiding this comment.
Similar to the parametrized test, asserting that the return code rc is 0 ensures that the command executed successfully and prevents false positives if the command fails due to other errors.
| assert "ERROR:json" not in err, err | |
| assert rc == 0, f"Command failed with exit code {rc}\nstdout: {out}\nstderr: {err}" | |
| assert "ERROR:json" not in err, err |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_bom_compat.py (1)
58-58: 💤 Low valueOptional: Mark unused unpacked variables with underscores.
The return values
rcandoutfrom_run()are unpacked but never used. Consider prefixing them with underscores to indicate they are intentionally unused, which silences the linter warning.♻️ Optional cleanup for linter warnings
- rc, out, err = _run(chart_type, path) + _rc, _out, err = _run(chart_type, path)And similarly at line 71:
- rc, out, err = _run("bar", path) + _rc, _out, err = _run("bar", path)Also applies to: 71-71
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_bom_compat.py` at line 58, The variables rc and out returned from _run(...) are unpacked but unused; update the unpacking to mark them as intentionally unused (e.g., rename to _rc and _out or prefix with an underscore) while keeping err if it is used (or rename all unused returns to _err/_out/_rc as appropriate), and apply the same change for the other _run(...) call; locate the calls to _run in this test module (symbol: _run) and adjust the left-hand tuple to use underscore-prefixed names to silence the linter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_bom_compat.py`:
- Line 58: The variables rc and out returned from _run(...) are unpacked but
unused; update the unpacking to mark them as intentionally unused (e.g., rename
to _rc and _out or prefix with an underscore) while keeping err if it is used
(or rename all unused returns to _err/_out/_rc as appropriate), and apply the
same change for the other _run(...) call; locate the calls to _run in this test
module (symbol: _run) and adjust the left-hand tuple to use underscore-prefixed
names to silence the linter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96fc1ff9-ee71-4ee4-9c84-6e21afb17ebf
📒 Files selected for processing (4)
cli_charts/cmd/_helpers.pycli_charts/dashboard.pytests/__init__.pytests/test_bom_compat.py
1 participant
PR: fix: read --file inputs with utf-8-sig to strip UTF-8 BOM
Summary
On Windows,
Out-File -Encoding utf8(and several other Windows tools) write aUTF-8 BOM (
\xef\xbb\xbf) by default. Python'sjson.loadtreats the BOM asinvalid content, raising:
This breaks every
--fileinput on Windows that was authored by PowerShell,Notepad's "Save as UTF-8", Excel's CSV export, or any tool that defaults to
UTF-8-with-BOM. The fix is a one-line change at each call site: switch
encoding='utf-8'(or the implicit default) toencoding='utf-8-sig', whichauto-strips a leading BOM and is fully backward-compatible with normal
no-BOM files.
Why I hit it
I was using
glyph-arts chart bar --file data.jsonfrom a PowerShell testharness. The harness wrote
data.jsonwithOut-File -Encoding utf8, whichsilently added a BOM. Every chart type (bar, sparkline, line, pie, gauge,
table) returned
ERROR:json: Expecting value: line 1 column 1 (char 0)eventhough the file content was valid JSON. Two hours of debugging later, hexdump
of the file showed the BOM.
Changes
9 call sites in 2 files:
cli_charts/cmd/_helpers.pyopen(args.file, encoding='utf-8')→'utf-8-sig'(4 sites) andopen(args.file)→open(args.file, encoding='utf-8-sig')(4 sites)cli_charts/dashboard.pyopen(args.file)→open(args.file, encoding='utf-8-sig')Plus
tests/test_bom_compat.py(new, 8 tests) covering:bar,sparkline,line,pie,gaugeopen(..., encoding='utf-8-sig')strips BOMencoding='utf-8'fails on a BOM file (proves the bug existed)Diff
Test results
Backward compatibility
encoding='utf-8-sig'is a strict superset ofencoding='utf-8':encoding='utf-8'.No existing user should see a behavior change unless they were already
working around the BOM bug (e.g. stripping the BOM with
sedor runningthe file through
iconvfirst).Reproduction
Notes
--strip-bomflag, bututf-8-sigmakes thatredundant — the fix is invisible to users.
--data(string) inputs to strip BOM too, butthe BOM only appears in file content, and
--datais already in-memoryby the time it reaches Python.
dashboard.pyhas areconfigure(encoding="utf-8")call onthe output stream (line 20-22). I left that alone — it addresses an
unrelated Windows console display issue, not the input BOM issue.
Happy to split this into multiple commits, narrow the scope to just
_helpers.py, or add a flag if you'd prefer a more conservative change.