Page MenuHomePhabricator

filemerge: byteify the open() mode
Needs ReviewPublic

Authored by mharbison72 on Sun, Nov 24, 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.Sun, Nov 24, 12:45 AM
dlax added a subscriber: dlax.Sun, Nov 24, 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.)