Clarify documentation on reconnect_delay #2769
Conversation
pymodbus/client/serial.py
Outdated
| @@ -35,8 +35,8 @@ class AsyncModbusSerialClient(ModbusBaseClient): | |||
| :param stopbits: Number of stop bits 1, 1.5, 2. | |||
| :param handle_local_echo: Discard local echo from dongle. | |||
| :param name: Set communication name, used in logging | |||
| :param reconnect_delay: Minimum delay in seconds.milliseconds before reconnecting. | |||
| :param reconnect_delay_max: Maximum delay in seconds.milliseconds before reconnecting. | |||
| :param reconnect_delay: Minimum delay in seconds before reconnecting. | |||
There was a problem hiding this comment.
This does not say that fractions are accepted ! We have had issues with a very similar text, where users said it was not clear that milliseconds could be used.
There was a problem hiding this comment.
I do not mind a new wording, but it needs to state what is possible, not everybody make the connection between seconds and a float.
There was a problem hiding this comment.
Ups forgot this change should be done for all client types, since it is the same parameter.
There was a problem hiding this comment.
This change brought it in line with how the timeout parameter is documented. I've also never seen it being documented differently in another library, hence my confusion when I misread it the way it is currently written.
I've now added (float) after each parameter description, including the 'timeout' value where this also was not specified.
pymodbus/client/serial.py
Outdated
| :param reconnect_delay: Minimum delay in seconds before reconnecting. | ||
| :param reconnect_delay_max: Maximum delay in seconds before reconnecting. | ||
| :param timeout: Timeout for connecting and receiving data, in seconds. | ||
| :param reconnect_delay: Minimum delay before reconnecting, in seconds (float). |
There was a problem hiding this comment.
This do not give the information a user is looking for.
much better would be something like (fractions are milliseconds).
There was a problem hiding this comment.
The current wording and the one that you are proposing now are both incorrect: the part behind the comma is a fraction of a second.
If you interpret the current wording seconds.milliseconds literally, 5 milliseconds can be written as 0.5 and 25 milliseconds as 0.25 , which is of course incorrect.
There was a problem hiding this comment.
I agree that the current interpretation is not mathematically correct, but (float) does not add useful information.
10 times (fraction of a second) is milliseconds...but I am not trying to be precise, just give users a hint, that milliseconds can be used.
Your issue was a good example you thought it was only milliseconds even though you knew it was a float.
There was a problem hiding this comment.
, but (float) does not add useful information.
At my university, data types are one of the first concepts covered in the Python Programming 101 course. "float" is a concept that every Python programmer should be familiar with: it clearly tells the programmer that the value is a floating-point number, or to be mathematically precise: a real number.
10 times (fraction of a second) is millisecond
I don't understand what you mean. This sentence does not make any sense.
but I am not trying to be precise, just give users a hint, that milliseconds can be used.
they can use any fraction of a second, not only milliseconds. Your 'hint' is currently confusing at best.
Your issue was a good example you thought it was only milliseconds even though you knew it was a float.
That is not a fair example. I started using this feature in my own library when migrating to pymodbus 3.0.0 . The documentation then read:
Your API_CHANGES.rst never mentioned that this unit had been changed!
There was a problem hiding this comment.
Are we in for a tug war, or do you want this PR merged.
1 second is 1000 milliseconds, so 3,125 is 3 seconds and 125 milliseconds or 3125 milliseconds if you prefer.
There was a problem hiding this comment.
Yes, I want this wording merged. I don't think there is a better way to describe these parameters.
There was a problem hiding this comment.
But as you pointed out, the type is float, so adding (float) does not give extra information. You could still describe the decimal part, which is milliseconds, I leave wording to you.
There was a problem hiding this comment.
But as you pointed out, the type is float, so adding (float) does not give extra information.
I've removed the (float) again
You could still describe the decimal part, which is milliseconds, I leave wording to you.
I don't agree:
- 0.5 is equal to 500 milliseconds
- 0.06 is equal to 60 milliseconds
- 0.007 is equal to 7 milliseconds
- 0.0008 is equal to 0.8 milliseconds.
The decimal part is thus in no way, shape or form milliseconds.
Documentation must be as clear and concise as possible, "in seconds" is what I consider the clearest and least confusing way to state what you must define here. Adding any other unit to the explanation is confusing at best and wrong at worst.
You already had that exact wording in place for the regular timeout parameter. So this PR in effect now just brings the documentation of the 3 parameters in line with each other.
A fix for #2768: the syntax of
seconds.millisecondsjust does not make sense. As the arguments are typed as floats it is clear that you can pass fractional values.