Should non-list values be allowed for multiple writes?

[alexrudd2]

The type hints and tests for write_registers() in ModbusClientMixin indicate this method only accepts lists of values.

pymodbus/pymodbus/client/mixin.py

Lines 422 to 442 in d2c7c77

	def write_registers(
	self,
	address: int,
	values: List[Union[int, float, str]],
	slave: int = 0,
	**kwargs: Any
	) -> pdu_req_write.WriteMultipleRegistersResponse:
	"""Write registers (code 0x10).
	:param address: Start address to write to
	:param values: List of booleans to write
	:param slave: (optional) Modbus slave unit ID
	:param kwargs: (optional) Experimental parameters.
	:returns: A deferred response handle
	:raises ModbusException:
	"""
	return self.execute(
	pdu_req_write.WriteMultipleRegistersRequest(
	address, values, slave, **kwargs
	)
	)

However, for at least a decade (66bd765) the underlying WriteMultipleRegistersRequest will actually accept single values and convert to a list.

pymodbus/pymodbus/register_write_message.py

Lines 149 to 160 in d2c7c77

	def __init__(self, address=None, values=None, unit=None, **kwargs):
	"""Initialize a new instance.
	:param address: The address to start writing to
	:param values: The values to write
	"""
	super().__init__(unit=unit, **kwargs)
	self.address = address
	if values is None:
	values = []
	elif not hasattr(values, "__iter__"):
	values = [values]

(1) Should this functionality be retained for write_registers()?
If no, we're technically breaking backwards compatibility although it's unclear if anyone uses it. pymodbus itself does not appear to.
If yes, we should

• update docstrings
• Type hints become (Union[int, float, str, List[Union[int, float, str]]] Union[List[int], int)
• Add tests for this functionality

(2) Same for readwrite_registers()
(3) Same for write_multiple_coils() write_coils(values: Union[List[bool], bool])`

[janiversen]

I think we should continue to allow non-list values as well as lists. Homeassistant is one of the users who use write_registers() to write a single value. This is done so, because some devices only accept write_registers() and have not implemented write_register().

and yes same for 2 and 3.

[alexrudd2]

Does Homeassistant use write_registers(values=123) or write_registers(values=[123])?

[janiversen]

Just checked the code it actually uses [123] so demanding a list() is ok.

[alexrudd2]

Do you have access to GitHub advanced code search? I have requested access to the beta but don't have it yet. It would be best to search for other users of these functions.

Otherwise, I could raise DeprecationWarning for a while?

[alexrudd2]

( I think) any other users which use mypy will already throw errors because the type signature is List[Union[int, float, str]]

Also, I recently learned that Union[float, int] is redundant: https://stackoverflow.com/questions/50928592/mypy-type-hint-unionfloat-int-is-there-a-number-type

[janiversen]

No I do not have access, I prefer using google, since it gives a much broader perspective.

I am not sure where you would raise the warning, but it should not be in production code. You have lost me why that is needed. Just define it as list[int]

[janiversen]

We do not accept float (that is more than 1 register and thus more than 1 value) nor do we allow str.

[alexrudd2]

No I do not have access, I prefer using google, since it gives a much broader perspective.
I am not sure where you would raise the warning, but it should not be in production code. You have lost me why that is needed. Just define it as list[int]

int has been permitted for 10 years, and I am concerned there might be users that will break. Probably it's enough to list as an API change.

We do not accept float (that is more than 1 register and thus more than 1 value) nor do we allow str.

OK, then the original typing in #1056 was too broad, and can be narrowed to List[int]

[janiversen]

int have not been permitted for 10 years, I corrected an error in that function about 2 years ago :-)

Independent you forget that even though you add type hint, it is still only a hint, and not enforced, so it will break nothing for 99% of the users, and the 1% will understand they need to change.

Having written that have no problem accepting "int or list[int]", but float and str are simply wrong, and will not work with the current code (nor any future code).

[alexrudd2]

Independent you forget that even though you add type hint, it is still only a hint, and not enforced, so it will break nothing for 99% of the users, and the 1% will understand they need to change.

Sorry, I should have been explicit. I intended to remove these lines that allow int.

pymodbus/pymodbus/register_write_message.py

Lines 159 to 160 in 5d7df32

	elif not hasattr(values, "__iter__"):
	values = [values]

[janiversen]

If they do not cause you a mypy problem, then let's leave them, otherwise I am quite ok with removing them.

[alexrudd2]

GitHub approved my code search beta. I did not search thoroughly, but found what appears to be at least one user of non-List values. So instead I think it's better to change the type hints, docstrings, and tests.

https://github.com/binsentsu/home-assistant-solaredge-modbus/blob/4f11978e470490b85f65cceb30fb52874d4f7750/custom_components/solaredge_modbus/select.py#L123-L126

[janiversen]

my my .... you came across HomeAssistant (although a custom component, with which I have had my challenges).