Page MenuHomePhabricator

filemerge: byteify the open() mode
AbandonedPublic

Authored by mharbison72 on Nov 24 2019, 12:45 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Summary

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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

mharbison72 created this revision.Nov 24 2019, 12:45 AM
dlax added a subscriber: dlax.Nov 24 2019, 6:51 AM

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()).

In D7517#110547, @dlax wrote:

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.

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.)

I'm a little fuzzy on this: should I see some test failures? or...?

I'm a little fuzzy on this: should I see some test failures? or...?

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.

This have been around for a while. What should we do with it?

This have been around for a while. What should we do with it?

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.)

marmoute accepted this revision.Feb 5 2020, 6:26 PM

I am leaning toward taking it for consistency (as @mharbison72 said) however, I also seems fine to simply abandon it.

mharbison72 abandoned this revision.Feb 8 2020, 11:49 PM

The py3 breakage fixed by D8099 convinced me that this is going in the wrong direction.