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 encoding option or binary mode for open() #9608

Merged
merged 2 commits into from
Feb 18, 2021
Merged

Use encoding option or binary mode for open() #9608

merged 2 commits into from
Feb 18, 2021

Conversation

methane
Copy link
Contributor

@methane methane commented Feb 13, 2021

No description provided.

@@ -766,7 +766,7 @@ def _generate_file(path, **kwargs):
# Record the REQUESTED file
if requested:
requested_path = os.path.join(dest_info_dir, 'REQUESTED')
with open(requested_path, "w"):
with open(requested_path, "wb"):
Copy link
Member

Choose a reason for hiding this comment

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

As this simply opens then closes (creating an empty file), this makes no real difference. I'm inclined to prefer not to use "b" because that implies the file is binary, and it isn't. But I don't really care much.

pathlib.touch might be better, now we don't have to worry about Python 2 compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer binary mode because creating TextIOWrapper is waste of time/memory and using locale-specific encoding is root of bugs.
But I respect your preference.

Copy link
Member

@pfmoore pfmoore Feb 13, 2021

Choose a reason for hiding this comment

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

That's a good point I hadn't considered.

Actually, I went looking to see what encoding the REQUESTED file should use. https://packaging.python.org/specifications/recording-installed-packages/ says it's tool-specific, and refers to PEP 376 which says it can be empty or contain a comment. So the content is essentially irrelevant, and binary mode therefore makes sense.

Let's use binary mode, and add a comment saying "Create an empty file (see PEP 376)", just to be clear that the encoding isn't relevant.

src/pip/_internal/self_outdated_check.py Show resolved Hide resolved
@pfmoore pfmoore added the skip news Does not need a NEWS file entry (eg: trivial changes) label Feb 13, 2021
@uranusjr uranusjr merged commit 1783041 into pypa:master Feb 18, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants