Conversation
This reverts commit 1506e98.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #1653 +/- ##
=======================================
Coverage 99.29% 99.29%
=======================================
Files 57 60 +3
Lines 11638 11655 +17
=======================================
+ Hits 11556 11573 +17
Misses 82 82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Will need a corresponding PR to bellows, since it's the only library that uses counters. |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates from callback-based counters to an event-driven diagnostic statistics system in zigpy. The goal is to replace the existing counter infrastructure with events to reduce unnecessary logging and improve the architecture for availability and counter tracking.
- Removes the extensive Counter, CounterGroup, and CounterGroups classes from the state module
- Introduces a new event system with EventBase for handling event subscriptions and emissions
- Adds DiagnosticStatistic class that emits events when values change, replacing the old counter system
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| zigpy/state.py | Removes all counter-related classes (Counter, CounterGroup, CounterGroups) and their usage in State class |
| zigpy/event/event_base.py | Implements new EventBase class with event subscription, emission, and async task management |
| zigpy/event/diagnostic_statistic.py | Adds DiagnosticStatistic class that emits events on value changes |
| zigpy/event/init.py | Exports the new event system components |
| zigpy/device.py | Adds diagnostics dictionary using DiagnosticStatistic to Device class |
| zigpy/application.py | Adds diagnostics dictionary using DiagnosticStatistic to ControllerApplication class |
| tests/test_event.py | Comprehensive tests for the new event system and diagnostic statistics |
| tests/test_app_state.py | Removes all tests for the old counter system |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| callback(*args, **kwargs) | ||
|
|
||
| unsub = self.on_event(event_name, event_listener, with_context=with_context) | ||
| return unsub # noqa: RET504 |
There was a problem hiding this comment.
[nitpick] The comment # noqa: RET504 suggests suppressing a linting rule about unnecessary return statements. Consider restructuring the code to avoid needing this suppression by directly returning the unsubscribe function without assigning it to a variable first.
| return unsub # noqa: RET504 | |
| return self.on_event( | |
| event_name, async_event_listener, with_context=with_context | |
| ) | |
| def event_listener(*args, **kwargs) -> None: | |
| unsub() | |
| callback(*args, **kwargs) | |
| return self.on_event(event_name, event_listener, with_context=with_context) |
| callback(*args, **kwargs) | ||
|
|
||
| unsub = self.on_event(event_name, event_listener, with_context=with_context) | ||
| return unsub # noqa: RET504 |
There was a problem hiding this comment.
[nitpick] Similar to line 86, this comment suggests suppressing a linting rule. Consider restructuring to avoid the suppression by directly returning the unsubscribe function.
| return unsub # noqa: RET504 | |
| def make_async_event_listener(unsub): | |
| async def async_event_listener(*args, **kwargs) -> None: | |
| unsub() | |
| task = asyncio.create_task(callback(*args, **kwargs)) | |
| self._event_tasks.append(task) | |
| task.add_done_callback(self._event_tasks.remove) | |
| return async_event_listener | |
| return self.on_event( | |
| event_name, | |
| make_async_event_listener, | |
| with_context=with_context, | |
| ) | |
| def make_event_listener(unsub): | |
| def event_listener(*args, **kwargs) -> None: | |
| unsub() | |
| callback(*args, **kwargs) | |
| return event_listener | |
| return self.on_event(event_name, make_event_listener, with_context=with_context) |
| ) | ||
| handler = getattr(self, f"handle_{event.event.replace(' ', '_')}", None) | ||
| if handler is None: | ||
| _LOGGER.warning("Received unknown event: %s", event) |
There was a problem hiding this comment.
Using getattr with user-controlled input (event.event) to dynamically call methods could be a security risk if event.event contains malicious values. Consider validating the event.event value or using a whitelist of allowed handler names.
| _LOGGER.warning("Received unknown event: %s", event) | |
| event_name = event.event.replace(' ', '_') | |
| if event_name not in self.ALLOWED_EVENTS: | |
| _LOGGER.warning("Received unknown or disallowed event: %s", event) | |
| return | |
| handler = getattr(self, f"handle_{event_name}", None) | |
| if handler is None: | |
| _LOGGER.warning("No handler found for event: %s", event) |
| DIAGNOSTICS_STATISTIC_CHANGE_EVENT_TYPE: str = "diagnostic_statistics_change" | ||
|
|
||
|
|
||
| @dataclass(frozen=True) |
There was a problem hiding this comment.
The TODO comment references dropping Python 3.9 support, but given that your knowledge cutoff is January 2025 and it's currently August 2025, Python 3.9 may already be out of support. Consider checking if this TODO can be addressed now.
| @dataclass(frozen=True) | |
| DIAGNOSTICS_STATISTIC_CHANGE_EVENT_TYPE: str = "diagnostic_statistics_change" | |
| @dataclass(frozen=True, kw_only=True) |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
This is a simple PR to migrate the ZHA event base into zigpy. We will slowly get rid of callback listeners and replace them with events.
My goal is to move availability and counters to events this release, to remove the endless logging of ZHA scanning device objects for availability state changes.