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

fix windows utf8 encoding 1/3 #719

Merged
merged 7 commits into from
Jun 22, 2020
Merged

fix windows utf8 encoding 1/3 #719

merged 7 commits into from
Jun 22, 2020

Conversation

phaustin
Copy link
Contributor

@phaustin phaustin commented Jun 17, 2020

This is one of three pull requests that add encoding = "utf8" to all read and write statements
for jupyter-book and MyST-NB. To test, clone:

https://github.com/phaustin/jb_windows_test

Follow the instructions in:

https://github.com/phaustin/jb_windows_test/blob/master/Readme.md#test-for-utf8-encoding

to build the book with the environment.yml that specifies the three PRs.

The github action will display the finished section at:

https://phaustin.github.io/jb_windows_test/flat_test/about_py.html#testing-greek-characters

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

Hey @phaustin - thanks for taking a stab at this. I am a but confused by some of the changes in this PR, you seem to be adding and changing things that aren't strictly related to UTF-8 encoding. Can you clarify?

@@ -0,0 +1,59 @@
name: build-deploy-book
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused about the addition of this extra workflow - can you add a comment at the top describing what it is testing? Also rename to something more specific since build-deploy-book could apply to many of our other CI/CD jobs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted -- that was vestigal -- I moved it into https://github.com/phaustin/jb_windows_test
and forgot to delete

on:
push:
branches:
- utf8
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed above

ls -lrtd *


# Push the book's HTML to github-pages
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a reason we want a windows-specific docs deploy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the record, the motivation behind the complicated workflow is to allow me to debug separate book problems by rendering independent html trees to the same gh-pages. The
environment takes about 4 minutes to set up for windows, so be able to do both the
encoding debugging and the unrelated nested/flat table of contents debugging (coming
next) in the same environment is a timesaver.

@@ -5,7 +5,7 @@
from .directive.toc import TableofContents


__version__ = "0.7.1dev0"
__version__ = "0.7.1dev2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this being changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@@ -9,7 +9,7 @@
import subprocess
from sphinx.util.osutil import cd

from ..sphinx import build_sphinx
from ..jbsphinx import build_sphinx
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are you renaming this module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's an issue with vscode running on windows -- the local sphinx module
shadows the global sphinx install so that importing sphinx.utils fails.
This was the path of least resistance around that problem, happy to revert if
necessary and just keep this on another fork.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah I'd prefer not to rename the submodule if possible. You should be able to directly import anyway, ..sphinx should be treated differently from sphinx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it's definitely a problem with the vscode debugger -- there are a couple of vscode issues that I'll leave for someone who actually works with windows.

@choldgraf choldgraf merged commit 95a1d98 into jupyter-book:master Jun 22, 2020
@choldgraf
Copy link
Collaborator

tests are passing and the changes LGTM, thanks for updating the PR and for the improvements @phaustin !!

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.

2 participants