-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Replace open(file, 'r') with open(file)
#9591
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 += ( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 {} ' | ||
|
|
||
There was a problem hiding this comment.
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"orencoding="utf-8".Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
We should probably go through all calls to
open()andpathlib.Path.read_text()addencoding=somethingeverywhere. It may even be a good idea to addencoding=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 understandopen(filename, 'r')is exactly equivalent toopen(filename)in python3There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah,
encoding="ascii"withoutreplacewould 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.