Skip to content

fix(report): write HTML export as utf-8#705

Open
immu4989 wants to merge 1 commit into
MAIF:masterfrom
immu4989:hotfix/report-encoding-utf8
Open

fix(report): write HTML export as utf-8#705
immu4989 wants to merge 1 commit into
MAIF:masterfrom
immu4989:hotfix/report-encoding-utf8

Conversation

@immu4989

Copy link
Copy Markdown
Contributor

Description

export_and_save_report() writes the rendered HTML report via
open(output_file, "w") without an explicit encoding=, so the
file is written with locale.getpreferredencoding(False). On Windows
that's cp1252, which raises
UnicodeEncodeError: 'charmap' codec can't encode character ...
whenever the nbconvert HTML body contains non-Latin1 characters
(feature names with accents, user-supplied text in additional data,
dataset string values, etc.).

The reporter (@dalestee) diagnosed the issue and prescribed the fix in
the issue body. This PR applies that fix exactly: add
encoding="utf-8" to the open() call at
shapash/report/generation.py:93.

Grepped the rest of shapash/ for sibling sites — every other
write-side open() call is in binary mode and every read-side one
touches ASCII-only bundled JSON, so this is the only line that needs
to change.

Fixes #699

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Added test_export_and_save_report_writes_utf8 in
tests/integration_tests/test_report_generation.py. It mocks
HTMLExporter and the local open binding in
shapash.report.generation, calls export_and_save_report, and
asserts that every write-mode open() call carries
encoding="utf-8".

The test uses mock.patch rather than monkeypatching locale.getpreferredencoding,
because Python 3.12 resolves open()'s default encoding at a lower
level than the locale module exposes — monkeypatching locale no
longer changes the actual behavior. A contract test on the call
signature is the most reliable way to catch a future regression on
CI runners that default to UTF-8.

  • pytest tests/integration_tests/test_report_generation.py -q8 passed (7 pre-existing + 1 new)
  • ruff format --check shapash/report/generation.py tests/integration_tests/test_report_generation.py → clean

Test Configuration:

  • OS: macOS
  • Python version: 3.12.13
  • Shapash version: 2.8.1 (editable install of this branch)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard to understand areas — only a docstring on the new test linking the bug; the fix itself is one line
  • I have made corresponding changes to the documentation — N/A, no user-facing API change
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules — N/A, no dependent changes

export_and_save_report() called open(output_file, "w") without an explicit
encoding, so the file was written with locale.getpreferredencoding(False).
On Windows that's cp1252, which raises UnicodeEncodeError whenever the
nbconvert HTML body contains non-Latin1 characters (feature names with
accents, user-supplied text in additional data, etc.).

Add encoding="utf-8" to the open() call so the export is consistent across
platforms. Add a regression test that mocks HTMLExporter + the local open
binding to assert the encoding kwarg is passed, since monkeypatching
locale doesn't affect Python 3.12's internal default-encoding resolution.

Fixes MAIF#699
@guerinclement

Copy link
Copy Markdown
Collaborator

Hi @immu4989,
Thanks again for your contribution and your interest in shapash.

We have a wider ongoing work on the report generation to get rid of papermill/jupyter dependencies.
The fix for issue #699 will be shipped along the release of the feature.

I let your PR opened in the meantime but it will likely not be merged.

@immu4989

Copy link
Copy Markdown
Contributor Author

Thanks @guerinclement, that makes total sense , and good to hear #699 will be addressed as part of the wider rework.

Happy to leave the PR open as a one line stopgap in case a Windows user reports the bug again before the new report module lands. Feel free to cherry pick the encoding="utf-8" change (or close this out) at any point , whichever is cleaner for you.

Looking forward to seeing the refactor land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Encoding error on Windows when generating report

2 participants