Address late review for Renault integration (home-assistant#55230)

* Add type hints

* Fix isort

* tweaks to state attributes

* Move lambdas to regular functions

* Split CHECK_ATTRIBUTES into DYNAMIC_ATTRIBUTES and FIXED_ATTRIBUTES

* Clarify tests

* Fix typo

homeassistant/components/renault/renault_entities.py

@@ -63,11 +63,8 @@ def data(self) -> StateType:
     @property
     def extra_state_attributes(self) -> Mapping[str, Any] | None:
         """Return the state attributes of this entity."""
-        if self.entity_description.coordinator == "battery":
-            last_update = (
-                getattr(self.coordinator.data, "timestamp")
-                if self.coordinator.data
-                else None
-            )
-            return {ATTR_LAST_UPDATE: last_update}
+        if self.entity_description.coordinator == "battery" and self.coordinator.data:
+            timestamp = getattr(self.coordinator.data, "timestamp")
+            if timestamp:
+                return {ATTR_LAST_UPDATE: timestamp}
         return None

homeassistant/components/renault/sensor.py

@@ -102,20 +102,53 @@ def native_value(self) -> StateType:
         return self.entity_description.value_lambda(self)
 
 
-def _get_formatted_charging_status(
-    data: KamereonVehicleBatteryStatusData,
-) -> str | None:
+def _get_charge_mode_icon(entity: RenaultDataEntity[T]) -> str:
+    """Return the icon of this entity."""
+    if entity.data == "schedule_mode":
+        return "mdi:calendar-clock"
+    return "mdi:calendar-remove"
+
+
+def _get_charging_power(entity: RenaultDataEntity[T]) -> StateType:
+    """Return the charging_power of this entity."""
+    if entity.vehicle.details.reports_charging_power_in_watts():
+        return cast(float, entity.data) / 1000
+    return entity.data
+
+
+def _get_charge_state_formatted(entity: RenaultDataEntity[T]) -> str | None:
     """Return the charging_status of this entity."""
+    data = cast(KamereonVehicleBatteryStatusData, entity.coordinator.data)
     charging_status = data.get_charging_status() if data else None
     return charging_status.name.lower() if charging_status else None
 
 
-def _get_formatted_plug_status(data: KamereonVehicleBatteryStatusData) -> str | None:
+def _get_charge_state_icon(entity: RenaultDataEntity[T]) -> str:
+    """Return the icon of this entity."""
+    if entity.data == ChargeState.CHARGE_IN_PROGRESS.value:
+        return "mdi:flash"
+    return "mdi:flash-off"
+
+
+def _get_plug_state_formatted(entity: RenaultDataEntity[T]) -> str | None:
     """Return the plug_status of this entity."""
+    data = cast(KamereonVehicleBatteryStatusData, entity.coordinator.data)
     plug_status = data.get_plug_status() if data else None
     return plug_status.name.lower() if plug_status else None
 
 
+def _get_plug_state_icon(entity: RenaultDataEntity[T]) -> str:
+    """Return the icon of this entity."""
+    if entity.data == PlugState.PLUGGED.value:
+        return "mdi:power-plug"
+    return "mdi:power-plug-off"
+
+
+def _get_rounded_value(entity: RenaultDataEntity[T]) -> float:
+    """Return the icon of this entity."""
+    return round(cast(float, entity.data))
+
+
 SENSOR_TYPES: tuple[RenaultSensorEntityDescription, ...] = (
     RenaultSensorEntityDescription(
         key="battery_level",
@@ -133,17 +166,9 @@ def _get_formatted_plug_status(data: KamereonVehicleBatteryStatusData) -> str |
         data_key="chargingStatus",
         device_class=DEVICE_CLASS_CHARGE_STATE,
         entity_class=RenaultSensor[KamereonVehicleBatteryStatusData],
-        icon_lambda=lambda x: (
-            "mdi:flash"
-            if x.data == ChargeState.CHARGE_IN_PROGRESS.value
-            else "mdi:flash-off"
-        ),
+        icon_lambda=_get_charge_state_icon,
         name="Charge State",
-        value_lambda=lambda x: (
-            _get_formatted_charging_status(
-                cast(KamereonVehicleBatteryStatusData, x.coordinator.data)
-            )
-        ),
+        value_lambda=_get_charge_state_formatted,
     ),
     RenaultSensorEntityDescription(
         key="charging_remaining_time",
@@ -164,29 +189,17 @@ def _get_formatted_plug_status(data: KamereonVehicleBatteryStatusData) -> str |
         name="Charging Power",
         native_unit_of_measurement=POWER_KILO_WATT,
         state_class=STATE_CLASS_MEASUREMENT,
-        value_lambda=lambda x: (
-            cast(float, x.data) / 1000
-            if x.vehicle.details.reports_charging_power_in_watts()
-            else x.data
-        ),
+        value_lambda=_get_charging_power,
     ),
     RenaultSensorEntityDescription(
         key="plug_state",
         coordinator="battery",
         data_key="plugStatus",
         device_class=DEVICE_CLASS_PLUG_STATE,
         entity_class=RenaultSensor[KamereonVehicleBatteryStatusData],
-        icon_lambda=lambda x: (
-            "mdi:power-plug"
-            if x.data == PlugState.PLUGGED.value
-            else "mdi:power-plug-off"
-        ),
+        icon_lambda=_get_plug_state_icon,
         name="Plug State",
-        value_lambda=lambda x: (
-            _get_formatted_plug_status(
-                cast(KamereonVehicleBatteryStatusData, x.coordinator.data)
-            )
-        ),
+        value_lambda=_get_plug_state_formatted,
     ),
     RenaultSensorEntityDescription(
         key="battery_autonomy",
@@ -227,7 +240,7 @@ def _get_formatted_plug_status(data: KamereonVehicleBatteryStatusData) -> str |
         name="Mileage",
         native_unit_of_measurement=LENGTH_KILOMETERS,
         state_class=STATE_CLASS_TOTAL_INCREASING,
-        value_lambda=lambda x: round(cast(float, x.data)),
+        value_lambda=_get_rounded_value,
     ),
     RenaultSensorEntityDescription(
         key="fuel_autonomy",
@@ -239,7 +252,7 @@ def _get_formatted_plug_status(data: KamereonVehicleBatteryStatusData) -> str |
         native_unit_of_measurement=LENGTH_KILOMETERS,
         state_class=STATE_CLASS_MEASUREMENT,
         requires_fuel=True,
-        value_lambda=lambda x: round(cast(float, x.data)),
+        value_lambda=_get_rounded_value,
     ),
     RenaultSensorEntityDescription(
         key="fuel_quantity",
@@ -251,7 +264,7 @@ def _get_formatted_plug_status(data: KamereonVehicleBatteryStatusData) -> str |
         native_unit_of_measurement=VOLUME_LITERS,
         state_class=STATE_CLASS_MEASUREMENT,
         requires_fuel=True,
-        value_lambda=lambda x: round(cast(float, x.data)),
+        value_lambda=_get_rounded_value,
     ),
     RenaultSensorEntityDescription(
         key="outside_temperature",
@@ -269,9 +282,7 @@ def _get_formatted_plug_status(data: KamereonVehicleBatteryStatusData) -> str |
         data_key="chargeMode",
         device_class=DEVICE_CLASS_CHARGE_MODE,
         entity_class=RenaultSensor[KamereonVehicleChargeModeData],
-        icon_lambda=lambda x: (
-            "mdi:calendar-clock" if x.data == "schedule_mode" else "mdi:calendar-remove"
-        ),
+        icon_lambda=_get_charge_mode_icon,
         name="Charge Mode",
     ),
 )

tests/components/renault/__init__.py

@@ -1,6 +1,7 @@
 """Tests for the Renault integration."""
 from __future__ import annotations
 
+from types import MappingProxyType
 from typing import Any
 from unittest.mock import patch
 
@@ -10,6 +11,7 @@
 from homeassistant.components.renault.const import DOMAIN
 from homeassistant.config_entries import SOURCE_USER
 from homeassistant.const import (
+    ATTR_ICON,
     ATTR_IDENTIFIERS,
     ATTR_MANUFACTURER,
     ATTR_MODEL,
@@ -20,7 +22,7 @@
 from homeassistant.helpers import aiohttp_client
 from homeassistant.helpers.device_registry import DeviceRegistry
 
-from .const import MOCK_CONFIG, MOCK_VEHICLES
+from .const import ICON_FOR_EMPTY_VALUES, MOCK_CONFIG, MOCK_VEHICLES
 
 from tests.common import MockConfigEntry, load_fixture
 
@@ -64,6 +66,12 @@ def get_fixtures(vehicle_type: str) -> dict[str, Any]:
     }
 
 
+def get_no_data_icon(expected_entity: MappingProxyType):
+    """Check attribute for  icon for inactive sensors."""
+    entity_id = expected_entity["entity_id"]
+    return ICON_FOR_EMPTY_VALUES.get(entity_id, expected_entity.get(ATTR_ICON))
+
+
 async def setup_renault_integration_simple(hass: HomeAssistant):
     """Create the Renault integration."""
     config_entry = get_mock_config_entry()

tests/components/renault/const.py

@@ -41,13 +41,21 @@
     VOLUME_LITERS,
 )
 
-CHECK_ATTRIBUTES = (
+FIXED_ATTRIBUTES = (
     ATTR_DEVICE_CLASS,
-    ATTR_ICON,
-    ATTR_LAST_UPDATE,
     ATTR_STATE_CLASS,
     ATTR_UNIT_OF_MEASUREMENT,
 )
+DYNAMIC_ATTRIBUTES = (
+    ATTR_ICON,
+    ATTR_LAST_UPDATE,
+)
+
+ICON_FOR_EMPTY_VALUES = {
+    "sensor.charge_mode": "mdi:calendar-remove",
+    "sensor.charge_state": "mdi:flash-off",
+    "sensor.plug_state": "mdi:power-plug-off",
+}
 
 # Mock config data to be used across multiple tests
 MOCK_CONFIG = {

tests/components/renault/test_binary_sensor.py

@@ -6,22 +6,24 @@
 
 from homeassistant.components.binary_sensor import DOMAIN as BINARY_SENSOR_DOMAIN
 from homeassistant.components.renault.renault_entities import ATTR_LAST_UPDATE
-from homeassistant.const import STATE_OFF, STATE_UNAVAILABLE
+from homeassistant.const import ATTR_ICON, STATE_OFF, STATE_UNAVAILABLE
+from homeassistant.core import HomeAssistant
 from homeassistant.setup import async_setup_component
 
 from . import (
     check_device_registry,
+    get_no_data_icon,
     setup_renault_integration_vehicle,
     setup_renault_integration_vehicle_with_no_data,
     setup_renault_integration_vehicle_with_side_effect,
 )
-from .const import CHECK_ATTRIBUTES, MOCK_VEHICLES
+from .const import DYNAMIC_ATTRIBUTES, FIXED_ATTRIBUTES, MOCK_VEHICLES
 
 from tests.common import mock_device_registry, mock_registry
 
 
 @pytest.mark.parametrize("vehicle_type", MOCK_VEHICLES.keys())
-async def test_binary_sensors(hass, vehicle_type):
+async def test_binary_sensors(hass: HomeAssistant, vehicle_type: str):
     """Test for Renault binary sensors."""
     await async_setup_component(hass, "persistent_notification", {})
     entity_registry = mock_registry(hass)
@@ -43,12 +45,12 @@ async def test_binary_sensors(hass, vehicle_type):
         assert registry_entry.unique_id == expected_entity["unique_id"]
         state = hass.states.get(entity_id)
         assert state.state == expected_entity["result"]
-        for attr in CHECK_ATTRIBUTES:
+        for attr in FIXED_ATTRIBUTES + DYNAMIC_ATTRIBUTES:
             assert state.attributes.get(attr) == expected_entity.get(attr)
 
 
 @pytest.mark.parametrize("vehicle_type", MOCK_VEHICLES.keys())
-async def test_binary_sensor_empty(hass, vehicle_type):
+async def test_binary_sensor_empty(hass: HomeAssistant, vehicle_type: str):
     """Test for Renault binary sensors with empty data from Renault."""
     await async_setup_component(hass, "persistent_notification", {})
     entity_registry = mock_registry(hass)
@@ -70,15 +72,15 @@ async def test_binary_sensor_empty(hass, vehicle_type):
         assert registry_entry.unique_id == expected_entity["unique_id"]
         state = hass.states.get(entity_id)
         assert state.state == STATE_OFF
-        for attr in CHECK_ATTRIBUTES:
-            if attr == ATTR_LAST_UPDATE:
-                assert state.attributes.get(attr) is None
-            else:
-                assert state.attributes.get(attr) == expected_entity.get(attr)
+        for attr in FIXED_ATTRIBUTES:
+            assert state.attributes.get(attr) == expected_entity.get(attr)
+        # Check dynamic attributes:
+        assert state.attributes.get(ATTR_ICON) == get_no_data_icon(expected_entity)
+        assert ATTR_LAST_UPDATE not in state.attributes
 
 
 @pytest.mark.parametrize("vehicle_type", MOCK_VEHICLES.keys())
-async def test_binary_sensor_errors(hass, vehicle_type):
+async def test_binary_sensor_errors(hass: HomeAssistant, vehicle_type: str):
     """Test for Renault binary sensors with temporary failure."""
     await async_setup_component(hass, "persistent_notification", {})
     entity_registry = mock_registry(hass)
@@ -107,11 +109,11 @@ async def test_binary_sensor_errors(hass, vehicle_type):
         assert registry_entry.unique_id == expected_entity["unique_id"]
         state = hass.states.get(entity_id)
         assert state.state == STATE_UNAVAILABLE
-        for attr in CHECK_ATTRIBUTES:
-            if attr == ATTR_LAST_UPDATE:
-                assert state.attributes.get(attr) is None
-            else:
-                assert state.attributes.get(attr) == expected_entity.get(attr)
+        for attr in FIXED_ATTRIBUTES:
+            assert state.attributes.get(attr) == expected_entity.get(attr)
+        # Check dynamic attributes:
+        assert state.attributes.get(ATTR_ICON) == get_no_data_icon(expected_entity)
+        assert ATTR_LAST_UPDATE not in state.attributes
 
 
 async def test_binary_sensor_access_denied(hass):

tests/components/renault/test_init.py

@@ -6,11 +6,12 @@
 
 from homeassistant.components.renault.const import DOMAIN
 from homeassistant.config_entries import ConfigEntryState
+from homeassistant.core import HomeAssistant
 
 from . import get_mock_config_entry, setup_renault_integration_simple
 
 
-async def test_setup_unload_entry(hass):
+async def test_setup_unload_entry(hass: HomeAssistant):
     """Test entry setup and unload."""
     with patch("homeassistant.components.renault.PLATFORMS", []):
         config_entry = await setup_renault_integration_simple(hass)
@@ -26,7 +27,7 @@ async def test_setup_unload_entry(hass):
     assert config_entry.entry_id not in hass.data[DOMAIN]
 
 
-async def test_setup_entry_bad_password(hass):
+async def test_setup_entry_bad_password(hass: HomeAssistant):
     """Test entry setup and unload."""
     # Create a mock entry so we don't have to go through config flow
     config_entry = get_mock_config_entry()
@@ -44,7 +45,7 @@ async def test_setup_entry_bad_password(hass):
     assert not hass.data.get(DOMAIN)
 
 
-async def test_setup_entry_exception(hass):
+async def test_setup_entry_exception(hass: HomeAssistant):
     """Test ConfigEntryNotReady when API raises an exception during entry setup."""
     config_entry = get_mock_config_entry()
     config_entry.add_to_hass(hass)