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

Use io.open in notebookapp.py to fix #4303 #4348

Closed
wants to merge 3 commits into from
Closed

Use io.open in notebookapp.py to fix #4303 #4348

wants to merge 3 commits into from

Conversation

slel
Copy link

@slel slel commented Jan 16, 2019

Following the discussion at #4340, in Python 2 we need io.open
rather than open if using a file descriptor rather than a file name.

slel added 2 commits January 16, 2019 09:48
Following the discussion at #4340, in Python 2 we need `io.open`
rather than `open` if using a file descriptor rather than a file name.
@@ -1671,7 +1671,7 @@ def server_info(self):
def write_server_info_file(self):
"""Write the result of server_info() to the JSON file info_file."""
try:
with open(self.info_file, 'w') as f:
with io.open(self.info_file, 'w') as f:
Copy link
Member

Choose a reason for hiding this comment

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

Can you add encoding='utf-8' here? It's not the same problem, but it would be good to do it while we're looking at this code.

@takluyver takluyver added this to the 5.7.4 milestone Jan 16, 2019
@takluyver
Copy link
Member

The test failures are because of tornado 6.0a1. I've committed to 5.7.x to limit it to tornado 5.x; I'll close and reopen this pull request to test it again.

@takluyver takluyver modified the milestones: 5.7.4, 5.7.5 Jan 16, 2019
@takluyver takluyver closed this Jan 16, 2019
@takluyver takluyver reopened this Jan 16, 2019
@@ -1671,7 +1671,7 @@ def server_info(self):
def write_server_info_file(self):
"""Write the result of server_info() to the JSON file info_file."""
try:
with open(self.info_file, 'w') as f:
with io.open(self.info_file, 'w', encoding='utf-8') as f:
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I think I told you to change the wrong one. This one needs to stay as regular open(), because the JSON module on Python 2 produces bytes, not unicode.

I thought we were looking at this line, but it already uses io.open():

with io.open(self.browser_open_file, 'w', encoding='utf-8') as f:

Copy link
Author

Choose a reason for hiding this comment

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

If commit f8d7d40 has all that is needed, can you use only that commit
and leave out the further two commits in this branch when you merge?

Copy link
Member

Choose a reason for hiding this comment

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

Yup. I'll open a new PR so it re-runs the CI tests with just that.

Copy link
Member

Choose a reason for hiding this comment

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

See #4349

@slel
Copy link
Author

slel commented Jan 16, 2019

Grepping for with open and with io.open in the 5.7.x branch
gives about 30 of each... it might make sense to go through them
systematically and check whether more open should become io.open
and whether more encoding='utf-8' should be added.

$ grep -r "with open" .
$ grep -r "with io.open" .

$ grep -r "with open" . | wc -l
$ grep -r "with io.open" . | wc -l

@takluyver
Copy link
Member

Merged #4349

@takluyver takluyver closed this Jan 16, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants