| Submitter | phabricator |
|---|---|
| Date | Nov. 6, 2019, 11:24 p.m. |
| Message ID | <differential-rev-PHID-DREV-jc2oxczrbnmeywjmxqca-req@mercurial-scm.org> |
| Download | mbox | patch |
| Permalink | /patch/42854/ |
| State | New |
| Headers | show |
Comments
indygreg added a comment. This one makes me sad because of potential performance regressions due to call overhead. I'd almost rather rip out Python 2 support rather than land this. But I could be convinced if pytypes is as cool as you say... Is there no way to annotate the types here without the call overhead? Could we do something like an `if False` trick to define a `def bytestr()` with the proper annotations? Surely there's got to be a way... REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7296/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7296 To: durin42, #hg-reviewers Cc: indygreg, mercurial-devel
durin42 added a comment. I've filed a bug with the pytype team, I'll report back. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7296/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7296 To: durin42, #hg-reviewers Cc: indygreg, mercurial-devel
This revision now requires changes to proceed. indygreg added a comment. indygreg requested changes to this revision. I'm going to mark for revisions until we know more so this doesn't show up in the reviewable list. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7296/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7296 To: durin42, #hg-reviewers, indygreg Cc: indygreg, mercurial-devel
dlax added a comment.
class bytestr(bytes): # type: (Union[bytes,str]) -> bytestr
[...]
Does this work?
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7296/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7296
To: durin42, #hg-reviewers, indygreg
Cc: mjpieters, dlax, indygreg, mercurial-devel
This revision now requires changes to proceed. dlax added a comment. dlax requested changes to this revision. Rather `class bytestr(bytes): # type: Callable[[Union[bytes, str], bytestr]` as @yuya suggested in D7380 <https://phab.mercurial-scm.org/D7380>. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7296/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7296 To: durin42, #hg-reviewers, indygreg, dlax Cc: mjpieters, dlax, indygreg, mercurial-devel
dlax added a comment. black complains because inline comments have only one space before (esp. the first one produces a parse error). LGTM otherwise. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7296/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7296 To: durin42, #hg-reviewers, indygreg, dlax Cc: mjpieters, dlax, indygreg, mercurial-devel
This revision now requires changes to proceed. dlax added inline comments. dlax requested changes to this revision. INLINE COMMENTS > pycompat.py:294 > > + bytestr = _bytestr # type: Callable[[Union[bytes, str], bytestr] > + A `]` is missing before `, bytestr`. Then, it's also missing `typing` imports. But, even with this, I get: Invalid type comment: Callable[[Union[bytes, str]], bytestr] [invalid-type-comment] Name 'bytestr' is not defined I'm able to make this pass with: bytestr = _bytestr bytestr = bytestr # type: Callable[[Union[bytes, str]], bytestr] REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7296/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7296 To: durin42, #hg-reviewers, indygreg, dlax Cc: mjpieters, dlax, indygreg, mercurial-devel
indygreg added a comment. @durin42 I just queued most of the remaining patches in this series. This one still needs your attention, it appears. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7296/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7296 To: durin42, #hg-reviewers, indygreg, dlax Cc: mjpieters, dlax, indygreg, mercurial-devel
This revision now requires changes to proceed. dlax added a comment. dlax requested changes to this revision. Sorry, still not ok afaict :/ INLINE COMMENTS > pycompat.py:305 > > + bytestr = _bytestr # type: Callable[[Union[bytes, str], bytestr]] > + `]` is still at the wrong place, I think. Should be `Callable[[Union[bytes, str]], bytestr]`. Then I still get the "Name 'bytestr' is not defined" error mentioned above. > pycompat.py:414 > byterepr = repr > - bytestr = str > + bytestr = str # type: Callable[[Union[bytes, str], bytestr] > iterbytestr = iter same here about missing `]`. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7296/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7296 To: durin42, #hg-reviewers, indygreg, dlax Cc: mjpieters, dlax, indygreg, mercurial-devel
durin42 added a comment.
In D7296#109672 <https://phab.mercurial-scm.org/D7296#109672>, @dlax wrote:
> Sorry, still not ok afaict :/
So, I tried fixing this and it actually made things worse? dagparser.py no longer typechecks if I correct the syntax? Try the pytype invocation from the test at the end of the series and you'll see what I mean.
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7296/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7296
To: durin42, #hg-reviewers, indygreg, dlax
Cc: mjpieters, dlax, indygreg, mercurial-devel
dlax added a comment. dlax added a subscriber: yuja. In D7296#109683 <https://phab.mercurial-scm.org/D7296#109683>, @durin42 wrote: > In D7296#109672 <https://phab.mercurial-scm.org/D7296#109672>, @dlax wrote: > >> Sorry, still not ok afaict :/ > > So, I tried fixing this and it actually made things worse? dagparser.py no longer typechecks if I correct the syntax? Try the pytype invocation from the test at the end of the series and you'll see what I mean. Ok, trying `pytype mercurial/dagparser.py` I get a couple of those errors: File ".../mercurial/dagparser.py", line 172, in parsedag: Function bytestr.__init__ was called with the wrong arguments [wrong-arg-types] Expected: (self, ints: Iterable[int]) Actually passed: (self, ints: str) My point was that we need to keep "mercurial/pycompat.py" passing pytype before considering modules it depends on. Once the missing `type: ` is added and bytestr <-> _bytestr trick applied, it's okay but the error in dagparser.py persists... Looking closer at the error above, it mentions `bytestr.__init__`, not `__new__` (and there is in fact no type annotation for `__new__` in typeshed <https://github.com/python/typeshed/blob/34d68ab0a2117a08fa221d3a10884f35cacf2cdc/stdlib/2and3/builtins.pyi#L559>). So I suspect the "Callable" trick is not enough and we'd need a workaround similar to da925257 <https://phab.mercurial-scm.org/rHGda925257a39e5797e5b2e35ce1d68e923ea8ddf2> by @yuja . INLINE COMMENTS > pycompat.py:305 > > + bytestr = _bytestr # Callable[[Union[bytes, str]], bytestr] > + Now the `type: ` is missing, so the comment is ignored. Adding it, `pytype mercurial/pycompat.py` gives: mercurial/pycompat.py", line 305, in <module>: Invalid type comment: Callable[[Union[bytes, str]], bytestr] [invalid-type-comment] Name 'bytestr' is not defined hence the kind of trick I suggested in my first comment on this line. REPOSITORY rHG Mercurial CHANGES SINCE LAST ACTION https://phab.mercurial-scm.org/D7296/new/ REVISION DETAIL https://phab.mercurial-scm.org/D7296 To: durin42, #hg-reviewers, indygreg, dlax Cc: yuja, mjpieters, dlax, indygreg, mercurial-devel
dlax added a comment.
In D7296#109684 <https://phab.mercurial-scm.org/D7296#109684>, @dlax wrote:
> Looking closer at the error above, it mentions `bytestr.__init__`, not `__new__` (and there is in fact no type annotation for `__new__` in typeshed <https://github.com/python/typeshed/blob/34d68ab0a2117a08fa221d3a10884f35cacf2cdc/stdlib/2and3/builtins.pyi#L559>).
https://github.com/python/typeshed/issues/2630
REPOSITORY
rHG Mercurial
CHANGES SINCE LAST ACTION
https://phab.mercurial-scm.org/D7296/new/
REVISION DETAIL
https://phab.mercurial-scm.org/D7296
To: durin42, #hg-reviewers, indygreg, dlax
Cc: yuja, mjpieters, dlax, indygreg, mercurial-devel
Patch
diff --git a/mercurial/urllibcompat.py b/mercurial/urllibcompat.py --- a/mercurial/urllibcompat.py +++ b/mercurial/urllibcompat.py @@ -105,7 +105,7 @@ def quote(s, safe=r'/'): # bytestr has an __iter__ that emits characters. quote_from_bytes() # does an iteration and expects ints. We coerce to bytes to appease it. - if isinstance(s, pycompat.bytestr): + if isinstance(s, pycompat._bytestr): s = bytes(s) s = urllib.parse.quote_from_bytes(s, safe=safe) return s.encode('ascii', 'strict') diff --git a/mercurial/pycompat.py b/mercurial/pycompat.py --- a/mercurial/pycompat.py +++ b/mercurial/pycompat.py @@ -154,7 +154,7 @@ bytechr = struct.Struct(r'>B').pack byterepr = b'%r'.__mod__ - class bytestr(bytes): + class _bytestr(bytes): """A bytes which mostly acts as a Python 2 str >>> bytestr(), bytestr(bytearray(b'foo')), bytestr(u'ascii'), bytestr(1) @@ -208,7 +208,7 @@ """ def __new__(cls, s=b''): - if isinstance(s, bytestr): + if isinstance(s, _bytestr): return s if not isinstance( s, (bytes, bytearray) @@ -398,7 +398,7 @@ unicode = unicode bytechr = chr byterepr = repr - bytestr = str + _bytestr = str iterbytestr = iter maybebytestr = identity sysbytes = identity @@ -504,3 +504,8 @@ return tempfile.NamedTemporaryFile( mode, bufsize, suffix=suffix, prefix=prefix, dir=dir, delete=delete ) + + +def bytestr(s=b''): + # type: (Union[bytes,str]) -> _bytestr + return _bytestr(s)