Add type hints to twisted.internet.defer #1448
Conversation
This comment has been minimized.
This comment has been minimized.
This moves the documentation to where they are defined, which seems nice. And it enables explicit typing for handling _CONTINUE.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| gen: Union[ | ||
| Generator[Deferred[_T], object, None], | ||
| Coroutine[Deferred[_T], object, None], | ||
| ], |
There was a problem hiding this comment.
Repeating a comment from @mstojcevich in @glyph's suggestion PR here so we don't lose it:
Is None correct for the generator/coroutine return type for these? I think it should be Optional[object] since this handles pulling the value off of StopIteration(x) when the generator returns x.
And I don't think YieldType is necessarily a Deferred when gen is a Generator.
I think the same concerns apply to the original PR fwiw.
# Conflicts: # src/twisted/internet/base.py # src/twisted/internet/defer.py
…red-yielding coroutines, not Future-yielding coroutines.
|
Now that Twisted exports type hints, I'm seeing errors in my projects like: error: Returning Any from function declared to return "Deferred" [no-any-return]Because, for example, |
To be honest, I'm wondering if we should roll this back. Our type hints are in pretty rough shape, and are definitely going to be evolving over the next couple of releases. Is there a way to make this easily opt-in for mypy consumers? |
src/twisted/internet/defer.py
Outdated
| # type note: base class "Awaitable" defined the type as: | ||
| # Callable[[], Generator[Any, None, _DeferredResultT]] | ||
| __await__ = __iter__ # type: ignore[assignment] |
There was a problem hiding this comment.
I don't understand the issue being ignored here. Isn't this compatible with Awaitable.__await__ ? That would seem to be the whole point?
There was a problem hiding this comment.
Here is the full error message:
src/twisted/internet/defer.py:919:17: error: Incompatible types in assignment (expression has type "Callable[[], Deferred[_DeferredResultT]]", base class "Awaitable" defined the type as
"Callable[[], Generator[Any, None, _DeferredResultT]]") [assignment]So the issue seems to be that Awaitable expects __iter__ to return a Generator, and not any Iterable. We could make __await__ a generator function, but that kind of seems gratuitous?
There was a problem hiding this comment.
I agree that it's gratuitous, and if this were new code I'd probably say "let's just make it compatible" but given that I don't want any more runtime changes than necessary, can you just file a typeshed bug and link it here? Given that this works fine at runtime right now this does seem wrong.
There was a problem hiding this comment.
Sorry, by "this" I mean the stdlib __await__ stub, not our code
| @@ -1966,16 +2173,15 @@ def __init__(self, name, scheduler=None): | |||
| if scheduler is None: | |||
| from twisted.internet import reactor | |||
|
|
|||
| scheduler = reactor | |||
| scheduler = cast(IReactorTime, reactor) | |||
There was a problem hiding this comment.
Hmm. This cast seems like it's a problem, but probably not this PR's problem.
There was a problem hiding this comment.
The problem here is that mypy thinks the reactor is ModuleType. It should be fixed in another PR, though I'm not sure what more module voodoo we can do about it.
My guess is that it would really be better if the normal way to get the installed reactor was via a callable and not a weird module import that's really a reactor.
There was a problem hiding this comment.
Yeah I'd be very happy if Mypy just encouraged us to get rid of the gross from twisted.internet import reactor idiom entirely.
* trunk: (21 commits) Update after review. Update after review. Use self.failureException. Update assertions for .todo tests. Remove todo tests. Update PR template. Update after review. Push codecov coverage on Win and macOS. Cleanup. Show deps as part of normal tox run. Refactor after review. Fix test_maybeDeferredSyncFailure so that the called function returns a Failure instead of returning a Deferred with an errback, Add separate test for identical pass-through Remove composite action. Run pip freeze on each env. Fix tox name for non-ipv6. Add empty value. Use lists for matrix values. Fix matrix include. Refactor GHA to fix ipv6 and add nodeps tests. Add news fragment ...
Contributor Checklist:
tox -e black-reformatto format my patch to meet the Twisted Coding Standardreviewto the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.