Cleanup ModbusSerialServer.__init__ kwargs#2962
Conversation
janiversen
left a comment
janiversen
left a comment
There was a problem hiding this comment.
Please also update your dev, you are a couple of commits behind.
| stopbits: int = 1, | ||
| handle_local_echo: bool = False, | ||
| allow_multiple_devices: bool = False, | ||
| port: str = 0, # type: ignore[assignment] |
There was a problem hiding this comment.
We try to avoid ignoring type checking !
It is probably hard to find a device that is present on all supported platforms, which could be given as default....so maybe an empty string.
There was a problem hiding this comment.
We try to avoid ignoring type checking !
It is probably hard to find a device that is present on all supported platforms, which could be given as default....so maybe an empty string.
There doesn't have to be a default value, it can also be left as just port: str,. If you try calling without passing port it will fail (missing 1 required keyword-only argument: 'port').
That is unless having it fallback to an empty string is useful, but that would probably just raise an exception when pyserial can't open the port
f17e2ef to
4de913c
Compare
|
Looking at coverage (ubuntu 3.14 in CI), it seems you have introduced a line that is not being tested: "Required test coverage of 99.95% reached. Total coverage: 99.97%" Please correct that. False alarm !!!! this is NOT caused by this PR. |
|
Please do not misunderstand my review comments !! I like what you are doing, and all in favor of getting it done....including the other parameter changes you suggested. BUT when doing it, it should be done correctly, so it is inline with the rest of the code. btw. I think we have used kwargs as escape goat in several APIs. |
2 participants
ModbusSerialServer's__init__method describes all kwargs, but some of them are stored in**kwargs. This can be made clearer with type information by giving them explicit types and default values.