-
Notifications
You must be signed in to change notification settings - Fork 79
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
Fix typing, mostly related to async_generator #86
Conversation
from async_generator import asynccontextmanager as _asynccontextmanager # type: ignore | ||
from typing_extensions import ParamSpec | ||
_P = ParamSpec('_P') | ||
_T = TypeVar('_T') | ||
def asynccontextmanager(func: Callable[_P, AsyncIterator[_T]]) -> Callable[_P, AsyncContextManager[_T]]: # type: ignore | ||
return _asynccontextmanager(func) |
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.
#87 has a less-verbose fix for this for python3.7+
I suspect some combination of the two is desired for support for 3.6 with type information.
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.
i did test this with 3.6, though not with a 3.6 pyright (I don't think it would make a difference)
this basically just copies the exact signature that the contextlib version has, so theres no need for pyright to understand sys.version, I don't think.
maybe it would still be better to use sys.version instead of the try-except though.
i might just merge your PR into my fork, when i have some time.
Hi @laundmo and @sdwilsh. Thanks for opening these pull requests. I'll tackle both #86 and #87 in here since they cover the same underlying issue. Good catch on the type errors in general. 👍 I'm happy to merge this as-is. I guess the only open question is how to deal with the untyped |
@frederikaalund why not both? replace the |
You're right, we need both. 👍 I'll merge this as soon as that last change lands in this PR. |
I'll credit you both in |
@frederikaalund i don't have any more changes planned. Feel free to merge, or do you want me to merge the other PR into this one first? |
Well, one of the PRs are going to be in conflict. I'll merge this one first. 👍 Then @sdwilsh can update his PR accordingly. Thanks for your contribution to asyncio-mqtt. :) |
While running the "advanced" example from the readme with pyright, i noticed it was alerting me to some typing issues, relating to asynccontextmanager and
Client.__aexit__
.Pyright errors
The issue with
Client.__aexit__
was simple, the arguments were missing theOptional[]
around their type hint, and also need to useBaseException
instead ofException
.After a LOT of time trying to figure out what the issue wth asynccontextmanager was, i noticed that the compatibility import
async_generator
was missing type information, which caused pyright to fail at inferring theAsyncContextManager
type forClient.filtered_messages
andClient.unfiltered_messages
.I solved this by redefining the asynccontextmanager imported from
async_generator
with the proper typehints, and ignoring the type issues on those lines. This could potentially be moved to a type stub, though im not very comfortable working with them yet.After these changes there are no more issues running pyright on the advanced readme example.
MyPy still throws a few errors relating to SocketOption, but this is behaviour that has not changed.
I have tested running the advanced readme example on python 3.6 and with modifications to the example due to missing features it runs fine.