Improve types #2824
Improve types #2824
Conversation
examples/server_async.py
Outdated
| # Continuing, or use a sparse DataBlock which can have gaps | ||
| datablock = lambda : ModbusSparseDataBlock({0x00: 0, 0x05: 1}) # pylint: disable=unnecessary-lambda-assignment | ||
| elif args.store == "factory": # pragma: no cover | ||
| elif args.store == "factory" or True: # pragma: no cover # pylint: disable=condition-evals-to-constant |
There was a problem hiding this comment.
Just to make it explicit what the final/default value is
There was a problem hiding this comment.
You replace one pylint disable with another and make it more difficult to read.
Why does this need to be changed ? (and if changed it should be changed to something without a pylint disable.
There was a problem hiding this comment.
I had done it this way to make it clear the final conditional is == factory.
The alternative here is to just use a default else.
--- elif args.store == "factory" or True: # pragma: no cover # pylint: disable=condition-evals-to-constant
+++ else: # args.store == "factory"
| """Return a summary of the modbus plus statistics. | ||
| :returns: 54 16-bit words representing the status | ||
| :returns: An iterator over lists of 8-bit integers representing each statistic |
There was a problem hiding this comment.
I think this docstring has been wrong for a long time...
janiversen
left a comment
janiversen
left a comment
There was a problem hiding this comment.
A couple of comments.
examples/server_async.py
Outdated
| # Continuing, or use a sparse DataBlock which can have gaps | ||
| datablock = lambda : ModbusSparseDataBlock({0x00: 0, 0x05: 1}) # pylint: disable=unnecessary-lambda-assignment | ||
| elif args.store == "factory": # pragma: no cover | ||
| elif args.store == "factory" or True: # pragma: no cover # pylint: disable=condition-evals-to-constant |
There was a problem hiding this comment.
You replace one pylint disable with another and make it more difficult to read.
Why does this need to be changed ? (and if changed it should be changed to something without a pylint disable.
pymodbus/datastore/context.py
Outdated
| """ | ||
| self.single = single | ||
| self._devices = devices or {} | ||
| self._devices: dict = devices or {} |
There was a problem hiding this comment.
No need for type declaration, the None is removed.
The type should be added in line 157 "dict | None"
There was a problem hiding this comment.
Agreed line 157 is better. Upon looking close, nobody ever calls ModbusDeviceContext(devices = None) so changing the default value to {} is cleaner all around.
There was a problem hiding this comment.
Bah, pylint complains about {}: https://pylint.pycqa.org/en/latest/user_guide/messages/warning/dangerous-default-value.html
There was a problem hiding this comment.
Actually, this has revealed a tiny error in either the docstring or examples.
It's possible to pass Modbus*Context directly, as long as you pass single=True!
pymodbus/examples/custom_msg.py
Lines 122 to 131 in 431fb6c
pymodbus/transport/transport.py
Outdated
| return new_sslctx | ||
|
|
||
| def copy(self) -> CommParams: | ||
| def copy(self: CommParams) -> CommParams: |
There was a problem hiding this comment.
This looks wrong, self already have the correct type ??
There was a problem hiding this comment.
dataclasses.replace() on line 131 is.. imperfect. mypy is slightly too forgiving, but zuban is stricter.
See https://discuss.python.org/t/better-typing-for-copy-replace-functions-that-operate-on-dataclasses/105004/9
error: Argument 1 to "replace" has incompatible type "Self"; expected a dataclass [misc]
This seemed like a good workaround. Other options:
(1) typing.Self (but that requires 3.11)
(2) an explicit cast - ugly
(3) Using copy.copy()
Make types either more explicit or clearer, to help more powerful type checkers than
mypy(I'm tryingpyrightandzuban)