Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ def read(rel_path):
here = os.path.abspath(os.path.dirname(__file__))
# intentionally *not* adding an encoding option to open, See:
# https://github.com/pypa/virtualenv/issues/201#issuecomment-3145690
with open(os.path.join(here, rel_path), 'r') as fp:
with open(os.path.join(here, rel_path)) as fp:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is OK for now. But it will fail when non-ASCII characters are added in README.rst or __init__.py.
Please add encoding="ascii" or encoding="utf-8".

Copy link
Member

@uranusjr uranusjr Feb 12, 2021

Choose a reason for hiding this comment

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

encoding="utf-8" is intentionally not addedd here for setuptools compatibility. setuptools does not write metadata with UTF-8, but platform encoding, so using UTF-8 here would break packaging.

One possible denominator is to use encoding="ascii", error="replace".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

https://docs.python.org/3/library/functions.html#open
The default encoding is platform dependent

We should probably go through all calls to open() and pathlib.Path.read_text() add encoding=something everywhere. It may even be a good idea to add encoding=locale.getpreferredencoding() where the dependence on the OS and configuration is intentional. I think this is out of scope for this PR. As far as I understand open(filename, 'r') is exactly equivalent to open(filename) in python3

Copy link
Contributor

Choose a reason for hiding this comment

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

If so, why not encoding="ascii"?

It prevents non-ASCII characters are added in README accidentally.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, encoding="ascii" without replace would make sense here. I’d prefer this be made into a separate PR, however, and include reasoning in that PR to make it more discoverable for future maintainers.

return fp.read()


Expand Down
2 changes: 1 addition & 1 deletion src/pip/_internal/req/constructors.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ def deduce_helpful_msg(req):
msg = " The path does exist. "
# Try to parse and check if it is a requirements file.
try:
with open(req, 'r') as fp:
with open(req) as fp:
Copy link
Contributor

Choose a reason for hiding this comment

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

This may fail on Windows if non-ASCII comments in the requirements file.
Please add encoding="utf-8".

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, the encoding parameter would be a backward-incompatible change, because historically pip has been reading the file with platform encoding. We can (and probably should at some point) switch to UTF-8 everywhere, but that needs to go through a deprecation cycle, and should be treated as a separate feature request.

# parse first line only
next(parse_requirements(fp.read()))
msg += (
Expand Down
2 changes: 1 addition & 1 deletion src/pip/_internal/req/req_uninstall.py
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,7 @@ def from_dist(cls, dist):

elif develop_egg_link:
# develop egg
with open(develop_egg_link, 'r') as fh:
with open(develop_egg_link) as fh:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is encoding of egg_link file. What happen if non-ASCII path on Windows?

Copy link
Member

Choose a reason for hiding this comment

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

It’s undefined because distutils is so ancient people didn’t care about this back then. Platform encoding is the backward compatible choice here.

link_pointer = os.path.normcase(fh.readline().strip())
assert (link_pointer == dist.location), (
'Egg-link {} does not match installed location of {} '
Expand Down
5 changes: 3 additions & 2 deletions tests/functional/test_install.py
Original file line number Diff line number Diff line change
Expand Up @@ -1298,7 +1298,7 @@ def test_install_log(script, data, tmpdir):
'install', data.src.joinpath('chattymodule')]
result = script.pip(*args)
assert 0 == result.stdout.count("HELLO FROM CHATTYMODULE")
with open(f, 'r') as fp:
with open(f) as fp:
# one from egg_info, one from install
assert 2 == fp.read().count("HELLO FROM CHATTYMODULE")

Expand All @@ -1321,7 +1321,8 @@ def test_cleanup_after_failed_wheel(script, with_wheel):
# One of the effects of not cleaning up is broken scripts:
script_py = script.bin_path / "script.py"
assert script_py.exists(), script_py
shebang = open(script_py, 'r').readline().strip()
with open(script_py) as f:
shebang = f.readline().strip()
assert shebang != '#!python', shebang
# OK, assert that we *said* we were cleaning up:
# /!\ if in need to change this, also change test_pep517_no_legacy_cleanup
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/test_no_color.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def get_run_output(option):
pytest.skip("Unable to capture output using script: " + cmd)

try:
with open("/tmp/pip-test-no-color.txt", "r") as output_file:
with open("/tmp/pip-test-no-color.txt") as output_file:
retval = output_file.read()
return retval
finally:
Expand Down
2 changes: 1 addition & 1 deletion tests/functional/test_vcs_bazaar.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ def test_export_rev(script, tmpdir):
url = hide_url('bzr+' + _test_path_to_file_url(source_dir) + '@1')
Bazaar().export(str(export_dir), url=url)

with open(export_dir / 'test_file', 'r') as f:
with open(export_dir / 'test_file') as f:
assert f.read() == 'something initial'


Expand Down
2 changes: 1 addition & 1 deletion tools/automation/release/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ def generate_news(session: Session, version: str) -> None:


def update_version_file(version: str, filepath: str) -> None:
with open(filepath, "r", encoding="utf-8") as f:
with open(filepath, encoding="utf-8") as f:
content = list(f)

file_modified = False
Expand Down