Skip to content

fix typing in add_listener and remove_listener #1335

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

Merged
merged 3 commits into from
Jun 22, 2022

Conversation

tamenol
Copy link
Contributor

@tamenol tamenol commented Jun 21, 2022

Use the union to typecheck these functions

Use the union to typecheck these functions
@codecov
Copy link

codecov bot commented Jun 21, 2022

Codecov Report

Merging #1335 (d431a3a) into develop (cb3d4dc) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #1335      +/-   ##
===========================================
+ Coverage    65.92%   65.99%   +0.06%     
===========================================
  Files           86       86              
  Lines         8955     8958       +3     
===========================================
+ Hits          5904     5912       +8     
+ Misses        3051     3046       -5     

@tamenol
Copy link
Contributor Author

tamenol commented Jun 21, 2022

Declaration of Listener

self.listeners: List[MessageRecipient] = list(listeners)

@zariiii9003
Copy link
Collaborator

Thank you. Since the notifier accepts coroutines, could you also add the Awaitable in the MessageRecipient definition?

MessageRecipient = Union[Listener, Callable[[Message], Union[Awaitable[None], None]]]

Then we can also remove the cast in

    def _on_message_received(self, msg: Message) -> None:
        for callback in self.listeners:
            res = cast(Union[None, Optional[Awaitable[Any]]], callback(msg))
            if res is not None and self._loop is not None and asyncio.iscoroutine(res):
                # Schedule coroutine
                self._loop.create_task(res)

@tamenol
Copy link
Contributor Author

tamenol commented Jun 21, 2022

Thank you. Since the notifier accepts coroutines, could you also add the Awaitable in the MessageRecipient definition?

MessageRecipient = Union[Listener, Callable[[Message], Union[Awaitable[None], None]]]

Then we can also remove the cast in

    def _on_message_received(self, msg: Message) -> None:
        for callback in self.listeners:
            res = cast(Union[None, Optional[Awaitable[Any]]], callback(msg))
            if res is not None and self._loop is not None and asyncio.iscoroutine(res):
                # Schedule coroutine
                self._loop.create_task(res)

Done :)

@zariiii9003
Copy link
Collaborator

Pylint complains about unused imports now. I will merge once that's fixed.

@tamenol
Copy link
Contributor Author

tamenol commented Jun 22, 2022

Pylint complains about unused imports now. I will merge once that's fixed.

Aha, I see. Will fix it. Still learning how to contribute.

Also for the if statement in _on_message_received, I use the walrus operator or leave as is?

    def _on_message_received(self, msg: Message) -> None:
        for callback in self.listeners:
            res = callback(msg)
            if res is not None and self._loop is not None and asyncio.iscoroutine(res):
                # Schedule coroutine
                self._loop.create_task(res)
    def _on_message_received(self, msg: Message) -> None:
        for callback in self.listeners:
            if (res := callback(msg)) is not None and self._loop is not None and asyncio.iscoroutine(res):
                # Schedule coroutine
                self._loop.create_task(res)

@zariiii9003
Copy link
Collaborator

The walrus operator would fail in python 3.7

@zariiii9003 zariiii9003 merged commit d330522 into hardbyte:develop Jun 22, 2022
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.

2 participants