Skip to content
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

Merged
merged 4 commits into from
Nov 9, 2021

Conversation

laundmo
Copy link
Contributor

@laundmo laundmo commented Nov 5, 2021

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
Argument of type "Client" cannot be assigned to parameter "cm" of type "AsyncContextManager[_T@enter_async_context]" in function "enter_async_context"
  "Client" is incompatible with protocol "AsyncContextManager[_T@enter_async_context]"
    "__aexit__" is an incompatible type
      Type "(exc_type: Type[Exception], exc: Exception, tb: TracebackType) -> Coroutine[Any, Any, None]" cannot be assigned to type "(__exc_type: Type[BaseException] | None, __exc_value: BaseException | None, __traceback: TracebackType | None) -> Awaitable[bool | None]"
        Parameter 1: type "Type[BaseException] | None" cannot be assigned to type "Type[Exception]"
          Type "Type[BaseException] | None" cannot be assigned to type "Type[Exception]"
        Parameter 2: type "BaseException | None" cannot be assigned to type "Exception"
          Type "BaseException | None" cannot be assigned to type "Exception"
        Parameter 3: type "TracebackType | None" cannot be assigned to type "TracebackType"
Argument of type "AsyncIterator[AsyncGenerator[MQTTMessage, None]]" cannot be assigned to parameter "cm" of type "AsyncContextManager[_T@enter_async_context]" in function "enter_async_context"
  "AsyncIterator[AsyncGenerator[MQTTMessage, None]]" is incompatible with protocol "AsyncContextManager[_T@enter_async_context]"
    "__aenter__" is not present
    "__aexit__" is not present
Argument of type "AsyncIterator[AsyncGenerator[MQTTMessage, None]]" cannot be assigned to parameter "cm" of type "AsyncContextManager[_T@enter_async_context]" in function "enter_async_context"
  "AsyncIterator[AsyncGenerator[MQTTMessage, None]]" is incompatible with protocol "AsyncContextManager[_T@enter_async_context]"
    "__aenter__" is not present
    "__aexit__" is not present

The issue with Client.__aexit__ was simple, the arguments were missing the Optional[] around their type hint, and also need to use BaseException instead of Exception.

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 the AsyncContextManager type for Client.filtered_messages and Client.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.

Comment on lines +33 to +38
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)
Copy link
Contributor

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.

Copy link
Contributor Author

@laundmo laundmo Nov 7, 2021

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.

@frederikaalund
Copy link
Collaborator

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 async_generator.asynccontextmanager in python 3.6. Again, I'll happily accept either of the approaches that you propose. Do you guys agree on a specific approach or do you need a BDFL decision? :)

@laundmo
Copy link
Contributor Author

laundmo commented Nov 9, 2021

@frederikaalund why not both? replace the try/except with the version if that @sdwilsh proposed, and leave my type overwrite in that. That way, 3.6 has correct typing and pyright can still understand it won't ever get to use async_generator.asynccontextmanager on versions > 3.6

@frederikaalund
Copy link
Collaborator

You're right, we need both. 👍 I'll merge this as soon as that last change lands in this PR.

@frederikaalund
Copy link
Collaborator

frederikaalund commented Nov 9, 2021

I'll credit you both in CHANGELOG.md by the way. :)

@laundmo
Copy link
Contributor Author

laundmo commented Nov 9, 2021

@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?

@frederikaalund
Copy link
Collaborator

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. :)

@frederikaalund frederikaalund merged commit 5d8a0f1 into empicano:master Nov 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants