Skip to content
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

Resolve default-encoding errors on Windows #187

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

AA-Turner
Copy link
Contributor

@AA-Turner AA-Turner commented Jul 16, 2024

See PYTHONWARNDEFAULTENCODING et al. Without this change I ran into charmap errors.

A

Comment on lines 525 to 527
with open(notebooks_dir / Path(notebook_unique_name), "w", encoding="utf-8") as f:
# nbf.write incorrectly formats multiline arrays in output.
json.dump(nb, f, indent=4, ensure_ascii=False)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative, given you're using Pathlib:

Suggested change
with open(notebooks_dir / Path(notebook_unique_name), "w", encoding="utf-8") as f:
# nbf.write incorrectly formats multiline arrays in output.
json.dump(nb, f, indent=4, ensure_ascii=False)
# nbf.write incorrectly formats multiline arrays in output.
nb_json = json.dump(nb, f, indent=4, ensure_ascii=False)
(notebooks_dir / notebook_unique_name).write_text(nb_json, encoding="utf-8")

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have nbformat as a dependency now. Can we make it write the notebook here instead of having to rely on JSON directly?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nbformat has been a dependency of the TryExamplesDirective from the beginning. This originally used nbformat to write the notebook, but as the comment mentions, there is an edge case it cannot handle. Arrays with multiple lines in output cells get formatted incorrectly when using nbformat to write the notebook.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the suggestion above, I think it should be

nb_json = json.dumps(nb, indent=4, ensure_ascii=False)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep sorry - I just made the suggestion via GH's editor, missed checking it properly.

A

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for the context, @steppi. I thought the problem might have been resolved by now, but that's something to look for in a different place.

Comment on lines 525 to 527
with open(notebooks_dir / Path(notebook_unique_name), "w", encoding="utf-8") as f:
# nbf.write incorrectly formats multiline arrays in output.
json.dump(nb, f, indent=4, ensure_ascii=False)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have nbformat as a dependency now. Can we make it write the notebook here instead of having to rely on JSON directly?

@steppi steppi added the bug Something isn't working label Jul 16, 2024
@steppi
Copy link
Collaborator

steppi commented Jul 16, 2024

Thanks for the patch @AA-Turner. This project enforces the black codestyle; to fix the test failure you just have to run black on the file jupyterlite_sphinx.py.

@AA-Turner
Copy link
Contributor Author

Hi -- this was just a quick fix as I'm debugging a new failure in Sphinx 7.4/SciPy -- I don't have the project checked out locally so can't easily run black/etc. Please feel free to push to this branch / whatever else is appropriate.

A

@steppi steppi merged commit 4074ee2 into jupyterlite:main Jul 18, 2024
5 checks passed
@steppi
Copy link
Collaborator

steppi commented Jul 18, 2024

Thanks again for the fix @AA-Turner. I've made the formatting change so in this goes.

@AA-Turner AA-Turner deleted the patch-1 branch July 18, 2024 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants