This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3536

json: reject unicode on py2 as well
ClosedPublic

Authored by martinvonz on May 12 2018, 12:35 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHGf5a1aa8c6987: json: reject unicode on py2 as well
Summary

This makes it consistent with the behavior on py3.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.May 12 2018, 12:35 AM
indygreg requested changes to this revision.May 12 2018, 12:43 AM
indygreg added a subscriber: indygreg.

unicode is not a type on Python 3. I think this should be changed to (str, pycompat.unicode). That will evaluate to (str, unicode) on Python 2 and (str, str) on Python 3. If you go this route, please rephrase the comment to denote the Python 2 behavior only triggering on unicode because of str == bytes.

This revision now requires changes to proceed.May 12 2018, 12:43 AM
yuja added a subscriber: yuja.May 12 2018, 1:52 AM
`unicode` is not a type on Python 3. I think this should be changed to `(str, pycompat.unicode)`.

Alternatively, it could be type(u''). I have no preference though.

martinvonz updated this revision to Diff 8657.May 12 2018, 2:12 AM
martinvonz added a comment.May 12 2018, 2:13 AM
In D3536#56022, @indygreg wrote:

unicode is not a type on Python 3. I think this should be changed to (str, pycompat.unicode). That will evaluate to (str, unicode) on Python 2 and (str, str) on Python 3. If you go this route, please rephrase the comment to denote the Python 2 behavior only triggering on unicode because of str == bytes.

Done, I believe.

yuja added a comment.May 12 2018, 2:52 AM
  • elif isinstance(obj, str):
  • # This branch is unreachable on Python 2, because bytes == str
  • # and we'll return in the next-earlier block in the elif
  • # ladder. On Python 3, this helps us catch bugs before they
  • # hurt someone.

+ elif isinstance(obj, (str, pycompat.unicode)):
+ # On Python 2, we'll only get here if obj is unicode, because
+ # str == bytes, so str is handled above.

Doh, I never thought we already had pycompat.unicode to be imported
automagically by the code transformer, and it's unavailable on Python 2!
Since we'll eventually ditch the transformer, we'll have to add
unicode = __builtin__.unicode to the py2 side of pycompat.

Nit: (str, can be removed as pycompat.unicode is str on Python 3.

martinvonz added a comment.May 16 2018, 1:49 PM
In D3536#56051, @yuja wrote:
  • elif isinstance(obj, str):
  • # This branch is unreachable on Python 2, because bytes == str
  • # and we'll return in the next-earlier block in the elif
  • # ladder. On Python 3, this helps us catch bugs before they
  • # hurt someone.

+ elif isinstance(obj, (str, pycompat.unicode)):
+ # On Python 2, we'll only get here if obj is unicode, because
+ # str == bytes, so str is handled above.

Doh, I never thought we already had pycompat.unicode to be imported
automagically by the code transformer, and it's unavailable on Python 2!
Since we'll eventually ditch the transformer, we'll have to add
unicode = __builtin__.unicode to the py2 side of pycompat.

Yep, tests caught that too (once I ran them :P). I switched to type(u'') instead and I'll leave the fix of pycompat out of this patch (since pycompat probably needs further changes for xrange and others anyway).

Nit: (str, can be removed as pycompat.unicode is str on Python 3.

Good point. I fixed that and removed the comment because it it didn't seem to add anything anymore.

martinvonz updated this revision to Diff 8717.May 16 2018, 1:49 PM
Closed by commit rHGf5a1aa8c6987: json: reject unicode on py2 as well (authored by martinvonz). · Explain WhyMay 17 2018, 8:12 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/templatefilters.py (10 lines)

Diff 8657

mercurial/templatefilters.py

elif obj is False: elif obj is False:
return 'false' return 'false'
elif obj is True: elif obj is True:
return 'true' return 'true'
elif isinstance(obj, (int, long, float)): elif isinstance(obj, (int, long, float)):
return pycompat.bytestr(obj) return pycompat.bytestr(obj)
elif isinstance(obj, bytes): elif isinstance(obj, bytes):
return '"%s"' % encoding.jsonescape(obj, paranoid=paranoid) return '"%s"' % encoding.jsonescape(obj, paranoid=paranoid)
elif isinstance(obj, str): elif isinstance(obj, (str, pycompat.unicode)):
# This branch is unreachable on Python 2, because bytes == str # On Python 2, we'll only get here if obj is unicode, because
# and we'll return in the next-earlier block in the elif # str == bytes, so str is handled above.
# ladder. On Python 3, this helps us catch bugs before they
# hurt someone.
raise error.ProgrammingError( raise error.ProgrammingError(
'Mercurial only does output with bytes on Python 3: %r' % obj) 'Mercurial only does output with bytes: %r' % obj)
elif util.safehasattr(obj, 'keys'): elif util.safehasattr(obj, 'keys'):
out = ['"%s": %s' % (encoding.jsonescape(k, paranoid=paranoid), out = ['"%s": %s' % (encoding.jsonescape(k, paranoid=paranoid),
json(v, paranoid)) json(v, paranoid))
for k, v in sorted(obj.iteritems())] for k, v in sorted(obj.iteritems())]
return '{' + ', '.join(out) + '}' return '{' + ', '.join(out) + '}'
elif util.safehasattr(obj, '__iter__'): elif util.safehasattr(obj, '__iter__'):
out = [json(i, paranoid) for i in obj] out = [json(i, paranoid) for i in obj]
return '[' + ', '.join(out) + ']' return '[' + ', '.join(out) + ']'