Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use coordinator for Button and Event #116408

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 18 additions & 4 deletions homeassistant/helpers/update_coordinator.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
from typing_extensions import TypeVar

from homeassistant import config_entries
from homeassistant.components.button import ButtonEntity
from homeassistant.components.event import EventEntity
from homeassistant.const import EVENT_HOMEASSISTANT_STOP
from homeassistant.core import CALLBACK_TYPE, Event, HomeAssistant, callback
from homeassistant.exceptions import (
Expand Down Expand Up @@ -465,13 +467,23 @@ def _async_refresh_finished(self) -> None:
class BaseCoordinatorEntity(entity.Entity, Generic[_BaseDataUpdateCoordinatorT]):
"""Base class for all Coordinator entities."""

_no_use_coordinator = False

def __init__(
self, coordinator: _BaseDataUpdateCoordinatorT, context: Any = None
) -> None:
"""Create the entity with a DataUpdateCoordinator."""
self.coordinator = coordinator
self.coordinator_context = context

def __init_subclass__(cls, **kwargs: Any) -> None:
"""Post initialisation processing."""
super().__init_subclass__(**kwargs)
# ButtonEntity and EventEntity provide their own states
# and should not register to use the coordinator for state updates.
if ButtonEntity in cls.__mro__ or EventEntity in cls.__mro__:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the coordinator should know about platforms. It's up to each integration to implement the coordinator entity or not do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comes from the side-discussion here in discord.
It just creates unnecessary overhead by registering listeners for entities which has no use of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integrations shouldn't use the coordinator entity if they shouldn't use it.

cls._no_use_coordinator = True

@property
def should_poll(self) -> bool:
"""No need to poll. Coordinator notifies entity of updates."""
Expand All @@ -480,11 +492,13 @@ def should_poll(self) -> bool:
async def async_added_to_hass(self) -> None:
"""When entity is added to hass."""
await super().async_added_to_hass()
self.async_on_remove(
self.coordinator.async_add_listener(
self._handle_coordinator_update, self.coordinator_context
# Only register for listener if needed
if self._no_use_coordinator is False:
self.async_on_remove(
self.coordinator.async_add_listener(
self._handle_coordinator_update, self.coordinator_context
)
)
)

@callback
def _handle_coordinator_update(self) -> None:
Expand Down
40 changes: 40 additions & 0 deletions tests/helpers/test_update_coordinator.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,13 @@
import requests

from homeassistant import config_entries
from homeassistant.components.button import ButtonEntity
from homeassistant.components.event import EventEntity
from homeassistant.const import EVENT_HOMEASSISTANT_STOP
from homeassistant.core import CoreState, HomeAssistant, callback
from homeassistant.exceptions import ConfigEntryNotReady
from homeassistant.helpers import update_coordinator
from homeassistant.helpers.entity import Entity
from homeassistant.util.dt import utcnow

from tests.common import MockConfigEntry, async_fire_time_changed
Expand Down Expand Up @@ -747,3 +750,40 @@ def listener():
unsub()
await crd.async_refresh()
assert len(last_update_success_times) == 1


@pytest.mark.parametrize(
"entity",
[ButtonEntity, EventEntity],
)
async def test_not_registered_listener(hass: HomeAssistant, entity: Entity) -> None:
"""Test not registering listener for ButtonEntity and EventEntity."""

async def refresh() -> int:
return 1

crd = update_coordinator.DataUpdateCoordinator[int](
hass,
_LOGGER,
name="test",
update_method=refresh,
update_interval=timedelta(seconds=10),
)
assert not crd._listeners

class TestEntity(update_coordinator.CoordinatorEntity, entity):
"""Representation of a Test entity."""

def __init__(
self, coordinator: update_coordinator.DataUpdateCoordinator[int]
) -> None:
"""Initialize the entity."""
super().__init__(coordinator)

test_entity = TestEntity(crd)
await test_entity.async_added_to_hass()

# Don't register listener for ButtonEntity and EventEntity
assert test_entity._no_use_coordinator is True
# Should not register any listener on coordinator
assert not crd._listeners