Improve the type hints #133
Conversation
asyncio_mqtt/client.py
Outdated
| ciphers: Optional[str] = None, | ||
| keyfile_password: Optional[str] = None | ||
| keyfile_password: Optional[ssl._PasswordType] = None, |
There was a problem hiding this comment.
ssl._PasswordType is private (leading underscore) so it may change. Let's not depend on it. Should they every expose it (without a leading underscore) then let's use it. 👍
There was a problem hiding this comment.
Same goes for ssl._SSLMethod above.
asyncio_mqtt/client.py
Outdated
| @@ -182,20 +188,22 @@ def __init__( | |||
| self._clean_start = clean_start | |||
| self._properties = properties | |||
| self._loop = asyncio.get_event_loop() | |||
| self._connected: "asyncio.Future[int]" = asyncio.Future() | |||
| self._disconnected: "asyncio.Future[Optional[int]]" = asyncio.Future() | |||
| self._connected: "asyncio.Future[Union[int, mqtt.ReasonCodes]]" = ( | |||
There was a problem hiding this comment.
I know that these types were wrapped in "" before already but is there any reason to do so? We don't need forward reference semantics here.
asyncio_mqtt/client.py
Outdated
| with self._pending_call(mid, cb_result, self._pending_subscribes): | ||
| # Wait for cb_result | ||
| return await self._wait_for(cb_result, timeout=timeout) | ||
| return await self._wait_for(cb_result.wait(), timeout=timeout) |
There was a problem hiding this comment.
This is a breaking change! Before we returned int (the ID). Now, we return None (since asyncio.Event.wait returns None).
Let's not make a breaking change at this point. I get the intent to simplify but I vote that we revert for now (to not break things). Indirectly, this changes the type back to asyncio.Future[int]
There was a problem hiding this comment.
The type annotation was wrong, the Future was of type int instead of the actual type Union[Tuple[int], List[mqtt.ReasonCodes]].
So, the return should be of type Union[Tuple[int], List[mqtt.ReasonCodes]].
It would be a breaking change to now really return an int instead of Union[Tuple[int], List[mqtt.ReasonCodes]].
There was a problem hiding this comment.
I agree. :) Most important part is that we keep the raw Future for now instead of an Event (the former holds a value wheras the latter doesn't). 👍
asyncio_mqtt/client.py
Outdated
| options: Optional[mqtt.SubscribeOptions] = None, | ||
| properties: Optional[Properties] = None, | ||
| timeout: int = 10, | ||
| ) -> int: |
There was a problem hiding this comment.
I like the idea to explicitly type these parameters. Makes it easier to use the library.
I know that you exhaust the current argument list for paho.Client.subscribe. In any case, let's keep the *args and **kwargs forwarding just for the sake of future compatibility. 👍 This way, if paho adds a new argument, asyncio-mqtt is still compatible (even though we don't expose explicit typings for ssaid argument).
Of course, this goes for the other functions as well.
asyncio_mqtt/client.py
Outdated
| @@ -555,7 +593,7 @@ def _on_unsubscribe( | |||
| userdata: Any, | |||
| mid: int, | |||
| properties: Optional[mqtt.Properties] = None, | |||
| reasonCodes: Optional[List[mqtt.ReasonCodes]] = None, | |||
| reasonCodes: Optional[List[mqtt.ReasonCodes] | mqtt.ReasonCodes] = None, | |||
There was a problem hiding this comment.
See above (about Union over | for now).
asyncio_mqtt/client.py
Outdated
| fut = self._pending_subscribes.pop(mid) | ||
| if not fut.done(): | ||
| fut.set_result(granted_qos) | ||
| self._pending_subscribes.pop(mid).set() |
There was a problem hiding this comment.
See above. Revert for now. It's not time for a breaking change. Also, the if may seem redundant but it actually avoids a race condition. Trust me. :)
- Add *args, **kwargs to subscribe, unsubscribe and publish - Remove forward reference - Remove private ssl types - Revert _pending_subscribes to asyncio.Future - Use Union instead of | - Use List and Tuple instead of list and tuple - Other minor changes
|
Should I squash and rebase the commits? |
|
Change |
I can (and probably will) do that through the GitHub UI so you don't have to. :)
Another good catch! Well done. 👍 I'm very happy with this PR. Thank you for your contribution to asyncio-mqtt! Keep it coming. 😄 |
|
Thanks |
Hi,
I formatted the code with black and isort. I could roll it back if you want.
I use types-paho-mqtt to update the type hints.
It pass mypy strict mode.