Page MenuHomePhabricator

filemerge: byteify the open() mode
Needs ReviewPublic

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

Details

Reviewers
None
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.