| indygreg | |
| dlax |
| hg-reviewers |
Credit to Yuya for the idea of annotating the class as though it was a
callable. I honestly didn't expect this to work, but it does!
We annotate this awkwardly on a name alias of the type because putting
the annotation directly on the class ...: line as a comment breaks
black and I don't want to go down that rabbit hole.
| Lint Skipped |
| Unit Tests Skipped |
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...
I'm going to mark for revisions until we know more so this doesn't show up in the reviewable list.
Rather class bytestr(bytes): # type: Callable[[Union[bytes, str], bytestr] as @yuya suggested in D7380.
black complains because inline comments have only one space before (esp. the first one produces a parse error).
LGTM otherwise.
| mercurial/pycompat.py | ||
|---|---|---|
| 305 | A ] is missing before , bytestr. Then, it's also missing typing imports. 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] | |
@durin42 I just queued most of the remaining patches in this series. This one still needs your attention, it appears.
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). So I suspect the "Callable" trick is not enough and we'd need a workaround similar to da925257 by @yuja .
| mercurial/pycompat.py | ||
|---|---|---|
| 305 | 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. | |
The status of this is unclear ? Are we waiting on:
This is the last series stuck at the bottom of yadda.
It feels like it is still a work in progress, @durin42 can you clarify the status of this and possibly move it out of need-review if it is still a work in progress ?
I'd like to get back to pytyping all the things, but I can't justify spending more time on it. If someone else wants to push things forward I'll gladly provide some mentorship.