-
Notifications
You must be signed in to change notification settings - Fork 950
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
eliminates implicit Optional #1817
Changes from all commits
96e6fc9
1563190
9128fad
fa75843
1288293
64ac7e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,7 @@ | |
import asyncio | ||
import socket | ||
from dataclasses import dataclass | ||
from typing import Any, Callable | ||
from typing import Any, Callable, cast | ||
|
||
from pymodbus.client.mixin import ModbusClientMixin | ||
from pymodbus.exceptions import ConnectionException, ModbusIOException | ||
|
@@ -22,7 +22,7 @@ class ModbusBaseClient(ModbusClientMixin, ModbusProtocol): | |
|
||
**Parameters common to all clients**: | ||
|
||
:param framer: (optional) Modbus Framer class. | ||
:param framer: Modbus Framer class. | ||
:param timeout: (optional) Timeout for a request, in seconds. | ||
:param retries: (optional) Max number of retries per request. | ||
:param retry_on_empty: (optional) Retry on empty response. | ||
|
@@ -53,20 +53,20 @@ class ModbusBaseClient(ModbusClientMixin, ModbusProtocol): | |
class _params: | ||
"""Parameter class.""" | ||
|
||
retries: int = None | ||
retry_on_empty: bool = None | ||
close_comm_on_error: bool = None | ||
strict: bool = None | ||
broadcast_enable: bool = None | ||
reconnect_delay: int = None | ||
retries: int | None = None | ||
retry_on_empty: bool | None = None | ||
close_comm_on_error: bool | None = None | ||
strict: bool | None = None | ||
broadcast_enable: bool | None = None | ||
reconnect_delay: int | None = None | ||
|
||
source_address: tuple[str, int] = None | ||
source_address: tuple[str, int] | None = None | ||
|
||
server_hostname: str = None | ||
server_hostname: str | None = None | ||
|
||
def __init__( # pylint: disable=too-many-arguments | ||
self, | ||
framer: type[ModbusFramer] = None, | ||
framer: type[ModbusFramer], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This changes the API, that is not something we do lightly ! I do not see the benefit of this change, please revert or explain why it is an advantage for the users. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, there is a type error in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is another problem, which I think is handled at lower levels in the code. Please do not change API, because this will cause every app to be changed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Apart from that the type error is quite easy to solve without changing the parameter itself. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I did not see how to resolve that easily. Here I need your support. |
||
timeout: float = 3, | ||
retries: int = 3, | ||
retry_on_empty: bool = False, | ||
|
@@ -122,10 +122,10 @@ def __init__( # pylint: disable=too-many-arguments | |
self.transaction = DictTransactionManager( | ||
self, retries=retries, retry_on_empty=retry_on_empty, **kwargs | ||
) | ||
self.reconnect_delay_current = self.params.reconnect_delay | ||
self.reconnect_delay_current = cast(float, self.params.reconnect_delay) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. both delay_current and reconnect_delay are (or should be float) so why the cast. cast is an ugly beast, that preferable never should be used, it typically hides a fundamental problem, likely maybe in this case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it should, that is the way to tell the lower system that reconnect is not used. That could of course be done differently, but please make 1 pull request for 1 problem, in this case "typing": There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, but if
|
||
self.use_udp = False | ||
self.state = ModbusTransactionState.IDLE | ||
self.last_frame_end: float = 0 | ||
self.last_frame_end: float | None = 0 | ||
self.silent_interval: float = 0 | ||
|
||
# ----------------------------------------------------------------------- # | ||
|
@@ -164,7 +164,7 @@ def idle_time(self) -> float: | |
return 0 | ||
return self.last_frame_end + self.silent_interval | ||
|
||
def execute(self, request: ModbusRequest = None) -> ModbusResponse: | ||
def execute(self, request: ModbusRequest | None = None) -> ModbusResponse: | ||
"""Execute request and get response (call **sync/async**). | ||
|
||
:param request: The request to process | ||
|
@@ -210,15 +210,15 @@ async def async_execute(self, request=None): | |
|
||
return resp | ||
|
||
def callback_data(self, data: bytes, addr: tuple = None) -> int: | ||
def callback_data(self, data: bytes, addr: tuple | None = None) -> int: | ||
"""Handle received data | ||
|
||
returns number of bytes consumed | ||
""" | ||
self.framer.processIncomingPacket(data, self._handle_response, slave=0) | ||
return len(data) | ||
|
||
def callback_disconnected(self, _reason: Exception) -> None: | ||
def callback_disconnected(self, _reason: Exception | None) -> None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can that really be None ? where is it called with None ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment 2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again I review your code...All call I can see have an exception. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In |
||
"""Handle lost connection""" | ||
for tid in list(self.transaction): | ||
self.raise_future( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -502,7 +502,7 @@ def read_fifo_queue(self, address: int = 0x0000, **kwargs: Any) -> ModbusRespons | |
# code 0x2B sub 0x0D: CANopen General Reference Request and Response, NOT IMPLEMENTED | ||
|
||
def read_device_information( | ||
self, read_code: int = None, object_id: int = 0x00, **kwargs: Any | ||
self, read_code: Union[int, None] = None, object_id: int = 0x00, **kwargs: Any | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Union ?? I think we should either use "Union" or "|" not both. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use I will mark the other redundant comments Resolved. |
||
) -> ModbusResponse: | ||
"""Read FIFO queue (code 0x2B sub 0x0E). | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -141,6 +141,7 @@ def __init__( | |
|
||
self.last_frame_end = None | ||
|
||
assert isinstance(self.comm_params.baudrate, int) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No assert in production code please. Apart from that this is a strange place to assert on an internal struct...I could understand (but not accept) if you did assert on a parameter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intention here was to narrow the type to an However, the better solution is to find where The change in transport.py ( |
||
self._t0 = float(1 + bytesize + stopbits) / self.comm_params.baudrate | ||
|
||
# Check every 4 bytes / 2 registers if the reading is ready | ||
|
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.
but framer is an optional parameter.
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.
Did you read my comment 3? If framer is optional and defaults to None then it should not be called unprotected in
__init__
:I don't think
None
can be called.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.
No I do not read comments, I review code. All optional parameters are described as optional, and framer is and should be optional.
None is a legal value for framer at API level.