Feature/clean typing linter #285

julesxxl · 2025-02-06T11:14:34Z

That was not easy... So no more errors with ruff check, ruff format, mypy. But there are the TODOs with pylint and two other problems.
Unfortunately, I didn't get it work in HA. Pfff... Anyway, the code is there and since it's not urgent, we'll have to understand what's wrong... So many changes...
(I rebased onto #281.)

Errors in the logs:

2025-02-06 11:32:08.536 WARNING (MainThread) [pymodbus.logging] BinaryPayloadDecoder is deprecated and will be removed in v3.9.0 !
Please use "client.convert_from_registers()" or "client.convert_to_registers"
See documentation: "https://pymodbus.readthedocs.io/en/latest/source/client.html#pymodbus.client.mixin.ModbusClientMixin.convert_from_registers"
2025-02-06 11:32:08.547 WARNING (MainThread) [pymodbus.logging] BinaryPayloadDecoder is deprecated and will be removed in v3.9.0 !
Please use "client.convert_from_registers()" or "client.convert_to_registers"
See documentation: "https://pymodbus.readthedocs.io/en/latest/source/client.html#pymodbus.client.mixin.ModbusClientMixin.convert_from_registers"
2025-02-06 11:32:08.552 WARNING (MainThread) [pymodbus.logging] BinaryPayloadDecoder is deprecated and will be removed in v3.9.0 !
Please use "client.convert_from_registers()" or "client.convert_to_registers"
See documentation: "https://pymodbus.readthedocs.io/en/latest/source/client.html#pymodbus.client.mixin.ModbusClientMixin.convert_from_registers"
2025-02-06 11:32:08.552 ERROR (MainThread) [custom_components.victron.coordinator] Unexpected error fetching victron data
Traceback (most recent call last):
  File "/Users/julienpichavant/homeassistant/lib/python3.13/site-packages/homeassistant/helpers/update_coordinator.py", line 380, in _async_refresh
    self.data = await self._async_update_data()
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/julienpichavant/.homeassistant/custom_components/victron/coordinator.py", line 104, in _async_update_data
    self.parse_register_data(
    ~~~~~~~~~~~~~~~~~~~~~~~~^
        data, register_info_dict[name], unit
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    ).items()
    ^
  File "/Users/julienpichavant/.homeassistant/custom_components/victron/coordinator.py", line 155, in parse_register_data
    raise DecodedataTypeUnsupported(
        f"Not supported data_type: {value.data_type}"
    )
custom_components.victron.coordinator.DecodedataTypeUnsupported: Not supported data_type: <custom_components.victron.const.STRING object at 0x11f0c4750>

sfstar · 2025-02-06T11:56:29Z

Thanks for the PR and effort, wil check out/review this weekend

custom_components/victron/button.py

custom_components/victron/binary_sensor.py

julesxxl · 2025-02-09T08:46:18Z

OK. Fixed those ones and others. 👍

sfstar · 2025-02-12T18:03:01Z

Great, wasn't able to finish the review this weekend.
Will look at the latest changes in the coming couple days

sfstar

Thank you for the PR @julesxxl.

I've added feedback and questions as they arose.
Once they have been processed I will run this version locally and do a final lookover.

Keep up the good work :)

sfstar · 2025-02-12T18:35:10Z

custom_components/victron/config_flow.py

        config = dict(self.config_entry.options)
-        user_input.pop(CONF_RESCAN, None)
+        if user_input:


TODO's run this locally and go through config flow

sfstar · 2025-02-12T18:37:12Z

custom_components/victron/const.py

@@ -75,100 +76,111 @@ def __init__(self, length=1, read_length=None):


 class EntityType:
-    """Base entityType."""
+    """Base entity_type."""


EntityType is an class here and should therefore not be renamed in the comment from class style to var style

sfstar · 2025-02-12T18:38:25Z

custom_components/victron/const.py

+        super().__init__(entity_type_name=entity_type_name)
+
+
+class GenericAlarmLedger(Enum):


Is this removed elsewhere?

removed from line 180 probably due to the use of class on 166 and needing to be declared before used

sfstar · 2025-02-12T18:40:45Z

custom_components/victron/const.py

@@ -207,7 +211,7 @@ class generic_alarm_ledger(Enum):
    "grid_L3_energy_reverse": RegisterInfo(
        2608, UINT16, UnitOfEnergy.KILO_WATT_HOUR, 100
    ),
-    "grid_serial": RegisterInfo(2609, STRING(7)),
+    "grid_serial": RegisterInfo(2609, str(STRING(7))),


Why cast the string as an str here?

sfstar · 2025-02-12T18:43:52Z

custom_components/victron/const.py

    ),
-    "battery_error": RegisterInfo(
-        register=1283, dataType=UINT16, entityType=TextReadEntityType(battery_error)
+    "BatteryError": RegisterInfo(


use batteryError here since generic Alarm ledger doesn't do the same as the battery error enum class

sfstar · 2025-02-12T19:01:34Z

custom_components/victron/hub.py

-        first_register = next(iter(registerInfoDict))
-        return registerInfoDict[first_register].register
+        first_register = next(iter(register_infoDict))
+        return register_infoDict[first_register].register


should the name be register_info_dict to conform to convention?

sfstar · 2025-02-12T19:05:04Z

custom_components/victron/number.py

+        if self.description.key:
+            full_key = str(self.description.slave) + "." + self.description.key
+        else:
+            full_key = str(self.description.slave) + "."


I believe this returns an invalid entity_id.

sfstar · 2025-02-12T19:05:27Z

custom_components/victron/popBuffer.py

+from typing import Any
+
+
+class PopBuffer:


Ignored in review as it is still being written

sfstar · 2025-02-12T19:09:17Z

custom_components/victron/sensor.py

@@ -120,7 +123,7 @@ def determine_victron_device_class(name, unit):
            SensorDeviceClass.VOLUME_STORAGE
        )  # Perhaps change this to water if only water is measured in volume units
    if unit in [member.value for member in UnitOfSpeed]:
-        if "meteo" in name:
+        if name and "meteo" in name:


add seperate check for name to exist and not check 2 times in different if statements.

sfstar · 2025-02-12T19:10:53Z

custom_components/victron/sensor.py

-                        self._attr_native_value = self.entity_type.decodeEnum(
+                    if data in {
+                        item.value  # type: ignore[union-attr]
+                        for item in self.entity_type.entity_type_name


has the decode enum been replaced?

sfstar and others added 3 commits January 30, 2025 20:05

add comment with 3.9.0 target

2077443

add popbuffer draft to eventually switchover to 3_9_0

9e455b3

Cleaning code (ruff check/format, pylint, mypy)

6078919

sfstar added enhancement Enhancement of the code, not introducing new features. refactor Improvement of existing code, not introducing new features. maintenance Generic maintenance tasks. labels Feb 6, 2025

sfstar reviewed Feb 7, 2025

View reviewed changes

custom_components/victron/button.py Outdated Show resolved Hide resolved

sfstar reviewed Feb 7, 2025

View reviewed changes

custom_components/victron/binary_sensor.py Outdated Show resolved Hide resolved

Clean more mixedCase

9e64831

port typing: int

c2ae5b2

sfstar reviewed Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/clean typing linter #285

Feature/clean typing linter #285

julesxxl commented Feb 6, 2025

sfstar commented Feb 6, 2025

julesxxl commented Feb 9, 2025

sfstar commented Feb 12, 2025

sfstar left a comment

sfstar Feb 12, 2025

sfstar Feb 12, 2025

sfstar Feb 12, 2025

sfstar Feb 12, 2025

sfstar Feb 12, 2025

sfstar Feb 12, 2025

sfstar Feb 12, 2025

sfstar Feb 12, 2025

sfstar Feb 12, 2025

sfstar Feb 12, 2025

sfstar Feb 12, 2025

		super().__init__(entity_type_name=entity_type_name)


		class GenericAlarmLedger(Enum):

Feature/clean typing linter #285

Are you sure you want to change the base?

Feature/clean typing linter #285

Conversation

julesxxl commented Feb 6, 2025

sfstar commented Feb 6, 2025

julesxxl commented Feb 9, 2025

sfstar commented Feb 12, 2025

sfstar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment