Replace open(file, 'r') with open(file)
#9591
Conversation
|
Tests failed on python==3.9 with INTERNALERROR. Passed on other versions |
|
Nudging CI to try again. |
uranusjr
added
the
skip news
| @@ -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: | |||
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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
There was a problem hiding this comment.
If so, why not encoding="ascii"?
It prevents non-ASCII characters are added in README accidentally.
There was a problem hiding this comment.
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.
| @@ -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: | |||
There was a problem hiding this comment.
This may fail on Windows if non-ASCII comments in the requirements file.
Please add encoding="utf-8".
There was a problem hiding this comment.
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.
| @@ -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: | |||
There was a problem hiding this comment.
What is encoding of egg_link file. What happen if non-ASCII path on Windows?
There was a problem hiding this comment.
It’s undefined because distutils is so ancient people didn’t care about this back then. Platform encoding is the backward compatible choice here.
Suggested by
pyupgrade --py3-plus