Add typing with mypy#791
Conversation
|
Tests are a bit flappy (not related to this PR, it's also happen on master or other PR). @akx did you also had unstable tests result for PR you created recently (i.e. did you re-run failed tests until they succeed) ? Because if you didn't had to re-run failed test, tests seems to fail more often recently. Edit: you already answered this question in #789 :) |
akx
left a comment
akx
left a comment
There was a problem hiding this comment.
Some softer suggestions and some actual bug-looking things here.
| assert msg.payload == expected_payload, f"Invalid payload: ({msg.payload})" | ||
| assert msg.qos == 1, f"Invalid qos: ({msg.qos})" | ||
| assert msg.retain is not False, f"Invalid retain: ({msg.retain})" | ||
| assert not msg.retain, f"Invalid retain: ({msg.retain})" |
There was a problem hiding this comment.
Isn't this the inverse test c.f. before?
There was a problem hiding this comment.
Yes, but I think the test was wrong.
- We don't produce a message with retain flag in this test, so it make sense that msg.retain is False
- Previously, msg.retain was an interger so
msg.retain is not Falsewas always true. - I've checked and the value was
0. So it seems expected to assert the valueFalse
There was a problem hiding this comment.
Maybe this change should be done in a separate PR, then? I'm afraid it will confuse future code archeologists terribly if this happens in a commit that says "Add typing".
|
Thank for the review. I've only fixed few of your feedback but it's bed time now, I'll continue tomorrow. |
akx
left a comment
akx
left a comment
There was a problem hiding this comment.
A couple more comments - sorry it took a while for the second round of reviews. Starting to look pretty nice though, excellent renovation here :) 🛠️
| mqtt_cs_new = 0 | ||
| mqtt_cs_connected = 1 | ||
| mqtt_cs_disconnecting = 2 | ||
| mqtt_cs_connect_async = 3 |
There was a problem hiding this comment.
These should maybe also be UPPER_SNAKE_CASE? The compatibility aliases in client.py can of course remain lower-case.
| import typing | ||
| from collections.abc import Iterable | ||
| from typing import Any, List, Tuple, Union |
There was a problem hiding this comment.
Might be good to import the typing one way only; I'd suggest the from typing way.
| return WebsocketWrapper(sock, self._host, self._port, self._ssl, | ||
| self._websocket_path, self._websocket_extra_headers) |
There was a problem hiding this comment.
Would maybe read easier as
| return WebsocketWrapper(sock, self._host, self._port, self._ssl, | |
| self._websocket_path, self._websocket_extra_headers) | |
| return WebsocketWrapper( | |
| sock, | |
| self._host, | |
| self._port, | |
| self._ssl, | |
| self._websocket_path, | |
| self._websocket_extra_headers, | |
| ) |
(and I'd also consider adding kwargs, so it's harder to accidentally pass things in the wrong order)
| def _create_socket_connection(self): | ||
| def _create_socket(self) -> SocketLike: | ||
| tcp_sock = self._create_socket_connection() | ||
| sock = self._create_ssl_socket_if_enable(tcp_sock) |
There was a problem hiding this comment.
I'm not sure the _if_enable is a good idea - I'd just do
| sock = self._create_ssl_socket_if_enable(tcp_sock) | |
| if self._ssl: | |
| sock = self._ssl_wrap_socket(tcp_sock) |
or similar? That would also simplify the return type of that function.
| if (hasattr(self._ssl_context, 'check_hostname') and | ||
| self._ssl_context.check_hostname): # type: ignore |
There was a problem hiding this comment.
| if (hasattr(self._ssl_context, 'check_hostname') and | |
| self._ssl_context.check_hostname): # type: ignore | |
| if getattr(self._ssl_context, 'check_hostname', False): # type: ignore |
| @@ -520,8 +636,9 @@ def __init__(self, client_id="", clean_session=None, userdata=None, | |||
| self._transport = transport.lower() | |||
| self._protocol = protocol | |||
| self._userdata = userdata | |||
| self._sock = None | |||
| self._sockpairR, self._sockpairW = (None, None,) | |||
| self._sock: socket.socket | WebsocketWrapper | ssl.SSLSocket | None = None | |||
There was a problem hiding this comment.
| self._sock: socket.socket | WebsocketWrapper | ssl.SSLSocket | None = None | |
| self._sock: SocketLike | None = None |
| reason = 132 # Unsupported protocol version | ||
| reason = ReasonCodes(CONNACK >> 4, aName="Unsupported protocol version") |
There was a problem hiding this comment.
This change doesn't seem strictly type-related - would be nice to be sure it's covered by tests, at least?
There was a problem hiding this comment.
I've done this after adding type to the on_connect callback and saw that reason could be either a int or a ReasonCodes. I believe using int was a mistake.
Anyway I've added a test, and test show that it should no break compatibility (reason == 132 work)
|
All your comments should be resolved. I think I'll squash-merge this PR due to multiple commits adding the removing thing. If you believe that we should kept multiple commits, I can do a rebase to only squash together part that belong to the same commit (but I want to squash or rebase, because their is change + exact revert of that change in this PR) |
2 participants
Split of #786 : contains add of mypy on client.py