| marmoute |
| hg-reviewers |
This is actually pycompat.open(), so it need bytes. It regressed recently on
default.
VSCode flagged some invalid mode to open() the other day, but I don't remember
where. That's what got me searching in this area. I'm almost certain that it
was the other way (i.e. saying open doesn't take bytes), but I can't find that
now.
| Lint Skipped |
| Unit Tests Skipped |
This is actually pycompat.open(), so it need bytes.
I don't understand why this is needed. The default value for "mode" as bytes comes from a407f9009392, but I don't understand the rationale.
Shouldn't we instead change all calls to pycompat.open() to use a native str for mode? (and drop sysstr(mode) in pycompat.open()).
The reason for explicitly marking the mode there was that sysstr(mode) was already in place. So that didn't change anything- mode was already required to be bytes.
Shouldn't we instead change all calls to pycompat.open() to use a native str for mode? (and drop sysstr(mode) in pycompat.open()).
I suspect the byte mode was done to be consistent with the file name being bytes? I agree with the first point though- I’d much rather this be an explicit call to pycompat.open() so that it’s clear that it isn’t the builtin function. I noticed that *attr() builtins are similarly replaced, but I didn’t look inside to see what they were about. (I’m assuming they’re also about bytes -> str.)
It looks like not. I glossed over the fact that pycompat.sysstr() will simply return str if given one, before trying to decode it. So it will take either bytes or str.
I looks like the VSCode complaint about passing bytes as the mode is only flagged when used in a context manager (several lines below), even though clicking through to the definition brings it to pycompat. I don't remember if this particular thing was flagged by static analysis, or it just caught my eye because 99% of the modes in other uses (outside tests/, contrib/ and setup.py) are bytes. But the latest version of PyCharm isn't complaining, nor is pytype. So I can abandon it if you'd prefer.
Since it's not a bug fix, I guess I can abandon it. I left it open as a reminder that there are inconsistencies with with how strings are passed to builtins, and am wondering if there's any interest in using str instead of bytes for things like this since it seems to occasionally confuse static analyzers. (Yes, I know this change goes the opposite way.)
I am leaning toward taking it for consistency (as @mharbison72 said) however, I also seems fine to simply abandon it.
The py3 breakage fixed by D8099 convinced me that this is going in the wrong direction.