This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2297

py3: backout 7c54917b31f6 to make sure second argument of open() is str
AbandonedPublic

Authored by pulkit on Feb 17 2018, 5:03 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Summary

This backouts changeset 7c54917b31f6 in which code to prevent adding b'' in
front of second argument of open() was deleted in favour of pycompat.open().
Looking back, that's looks like a wrong decision as replacing open() with
pycompat.open() is not a good idea because of the following reasons:

  • There are around 100 occurences of open() call, it's better if we can prevent replacing all these calls
  • In future, when we will dropping all the compatibility code, we don't have to take care of changing pycompat.open back to open

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pulkit created this revision.Feb 17 2018, 5:03 AM
yuja added a subscriber: yuja.Feb 17 2018, 5:16 AM

Can you elaborate why you think it wasn't a good idea to replace open()?

IMHO, it's as hackish as replacing '' with b''.

pulkit edited the summary of this revision. (Show Details)Feb 18 2018, 12:10 PM
pulkit added a comment.Feb 22 2018, 10:06 AM
In D2297#37960, @yuja wrote:

Can you elaborate why you think it wasn't a good idea to replace open()?
IMHO, it's as hackish as replacing '' with b''.

Added couple of points.

yuja added a comment.Feb 22 2018, 10:50 AM

"There are around 100 occurences of open() call, it's better if we can
prevent replacing all these calls
"In future, when we will dropping all the compatibility code, we don't have
to take care of changing pycompat.open back to open

Yeah, that's the point why we have from pycompat import open to compensate
the mess introduced by our code transformer. We want to keep 'rb' without b''.
We could do that by the transformer, but I think it's better to keep the
transformer less complicated.

FWIW, we'll need to port many of these 100 open() calls to vfs. I have vague
memory that Python 3.5 or 3.6 introduced another mess on Windows I/O around
ANSI vs Unicode.

pulkit abandoned this revision.Jun 14 2018, 4:03 PM
In D2297#39249, @yuja wrote:

"There are around 100 occurences of open() call, it's better if we can
prevent replacing all these calls
"In future, when we will dropping all the compatibility code, we don't have
to take care of changing pycompat.open back to open
Yeah, that's the point why we have from pycompat import open to compensate
the mess introduced by our code transformer. We want to keep 'rb' without b''.
We could do that by the transformer, but I think it's better to keep the
transformer less complicated.

I agree with you that we should keep transformer less complicated as much possible. Abandoning this revision.

Revision Contents
Changeset List

PathPackages
Mmercurial/__init__.py (9 lines)

Diff 5807

mercurial/__init__.py

# .encode() and .decode() on str/bytes/unicode don't accept # .encode() and .decode() on str/bytes/unicode don't accept
# byte strings on Python 3. # byte strings on Python 3.
elif fn in ('encode', 'decode') and _isop(i - 1, '.'): elif fn in ('encode', 'decode') and _isop(i - 1, '.'):
for argn in range(2): for argn in range(2):
argidx = _findargnofcall(argn) argidx = _findargnofcall(argn)
if argidx is not None: if argidx is not None:
_ensureunicode(argidx) _ensureunicode(argidx)
# Bare open call (not an attribute on something else), the
# second argument (mode) must be a string, not bytes
elif fn == 'open' and not _isop(i - 1, '.'):
arg1idx = _findargnofcall(1)
if arg1idx is not None:
_ensureunicode(arg1idx)
# It changes iteritems/values to items/values as they are not # It changes iteritems/values to items/values as they are not
# present in Python 3 world. # present in Python 3 world.
elif fn in ('iteritems', 'itervalues'): elif fn in ('iteritems', 'itervalues'):
yield t._replace(string=fn[4:]) yield t._replace(string=fn[4:])
continue continue
# Emit unmodified token. # Emit unmodified token.
yield t yield t
# Header to add to bytecode files. This MUST be changed when # Header to add to bytecode files. This MUST be changed when
# ``replacetoken`` or any mechanism that changes semantics of module # ``replacetoken`` or any mechanism that changes semantics of module
# loading is changed. Otherwise cached bytecode may get loaded without # loading is changed. Otherwise cached bytecode may get loaded without
# the new transformation mechanisms applied. # the new transformation mechanisms applied.
BYTECODEHEADER = b'HG\x00\x0a' BYTECODEHEADER = b'HG\x00\x0b'
class hgloader(importlib.machinery.SourceFileLoader): class hgloader(importlib.machinery.SourceFileLoader):
"""Custom module loader that transforms source code. """Custom module loader that transforms source code.
When the source code is converted to a code object, we transform When the source code is converted to a code object, we transform
certain patterns to be Python 3 compatible. This allows us to write code certain patterns to be Python 3 compatible. This allows us to write code
that is natively Python 2 and compatible with Python 3 without that is natively Python 2 and compatible with Python 3 without
making the code excessively ugly. making the code excessively ugly.