-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add hint for AsyncIterator incompatible return type #15883
Conversation
4fea3c5
to
2c8aaa9
Compare
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
@hauntsaninja OK on the approach? If so, I'll also add the same treatment to |
|
||
class Launcher(P): | ||
def launch(self) -> AsyncIterator[int]: # E: Return type "AsyncIterator[int]" of "launch" incompatible with return type "Coroutine[Any, Any, AsyncIterator[int]]" in supertype "P" \ | ||
# N: Consider declaring "launch" in supertype "P" without "async" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it instead say: "consider adding async
to def lauch()
"?
Because in my experience, AsyncIterators are way more common with async def
rather than regular def
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ehh, this is confusing :|
The 'problem' here should really be reported on this line:
class P(Protocol):
!> async def launch(self) -> AsyncIterator[int]:
raise BaseException
but we don't put notices proactively on declarations that smell funny, or do we?
The error on the derived Launcher.launch
gives us the excuse to make a notice. What we want to say is "listen, we must error on Launcher.launch
, but Launcher.launch
is probably fine, consider changing the base declaration instead".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I think test case might be little less confusing if it used yield to showcase the common issue, but should all work the same
For issue described in #5070 and documented in #14973, add a contextual link to the docs.