Tado change to async and add Data Update Coordinator #134175
Conversation
| device_info: dict[str, Any], | ||
| entity_description: TadoBinarySensorEntityDescription, | ||
| ) -> None: | ||
| """Initialize of the Tado Sensor.""" | ||
| self.entity_description = entity_description | ||
| self._tado = tado | ||
| super().__init__(device_info) | ||
| self._tado = coordinator |
There was a problem hiding this comment.
You're using a CoordinatorEntity, the coordinator is stored in self.coordinator by default
| def _async_update_zone_callback(self) -> None: | ||
| """Load tado data and update state.""" | ||
| self._async_update_zone_data() | ||
| super().async_write_ha_state() |
There was a problem hiding this comment.
So why do we do this write here? This would already be handled by _handle_coordinator_update right?
| self._async_update_home_data() | ||
| self._async_update_zone_data() |
There was a problem hiding this comment.
Any reason why these are split?
There was a problem hiding this comment.
Old code. I left it in to get a working prototype and this called at the beginning. I forgot to optimize this...
| username: str, | ||
| password: str, | ||
| fallback: str, | ||
| debug: bool = False, |
There was a problem hiding this comment.
Or just throw in the whole entry
| coordinator = TadoDataUpdateCoordinator( | ||
| hass=hass, username="mock", password="mock", fallback="NEXT_TIME_BLOCK" | ||
| ) |
There was a problem hiding this comment.
So we should not create our own coordinator in the tests. Instead we should test more end-to-end. So in this test you're testing setup exceptions. Rather mock the library, have HA setup the integration and then observe the config entry state
| async def test_coordinator_successful_setup( | ||
| hass: HomeAssistant, snapshot: SnapshotAssertion | ||
| ) -> None: |
There was a problem hiding this comment.
In this case, we can check the created entities if they are created correctly. This way we do not rely on the coordinator, or how the coordinator works, we just want to know that the integration is able to update when needed and is able to handle errors correctly.
There was a problem hiding this comment.
Added bonus. since we don't really want to assume that we use a coordinator, I dislike the test_coordinator since that assumes it. The mentioned test replacements can be placed in test_init.py and test_sensor.py for example
| @@ -13,15 +13,17 @@ | |||
| from homeassistant.core import HomeAssistant | |||
|
|
|||
|
|
|||
| def dummy_tado_connector(hass: HomeAssistant, fallback) -> TadoConnector: | |||
| def dummy_tado_connector(hass: HomeAssistant, fallback) -> TadoDataUpdateCoordinator: | |||
There was a problem hiding this comment.
I think in this case it would be nice if we can remove these tests soon
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Co-authored-by: Joost Lekkerkerker <[email protected]>
Breaking change
Proposed change
Upgrading the Tado integration to go with the better variant to utilize the Data Update Coordinator. In the same time, the synchronous calls have all been wrapped in the
async_add_executor_joband to make the Coordinator use async.TODO:
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: