-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX: Support UTF-8 encoding for JSON files #1357
Conversation
I don't think we want to convert to ASCII for any JSON we're writing to disk. Hence, we don't need a parameter to control this behavior :) |
This reverts commit 4c47679.
TIL: That json.loads will always convert unicode. So to test that unicode was properly encoded while writing to disk, I had to had to just read the text on disk without the json module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice adaptation of the existing test. One suggested improvement, see below
Instead of closing and re-opening the file, rewind the "playhead" to the start of the open file, then use fid.read() as usual Co-authored-by: Daniel McCloy <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1357 +/- ##
=======================================
Coverage 97.42% 97.42%
=======================================
Files 40 40
Lines 8918 8921 +3
=======================================
+ Hits 8688 8691 +3
Misses 230 230 ☔ View full report in Codecov by Sentry. |
Thanks @scott-huberty |
fixes #1356
_write_json
is used a lot in the MNE-BIDS codebase... I want to get your feedback on whether adding a new parameterensure_ascii=False
to_write_tsv
is the right way to go?Because I'm not sure if there are cases in MNE-BIDS where we do want to enforce ASCII encoding during JSON writing.. If so, we could change the default to
ensure_ascii=True
, and passFalse
explicitly for the cases where we want to support UTF-8?cc @sappelhoff @hoechenberger @larsoner
TODO