Remove unused _state attribute from binary sensors#821
Conversation
`BinarySensor._state` was only ever assigned, never read: the `state` property and `is_on` recompute directly from the cluster, and `maybe_emit_state_changed_event` compares `self.state`. Drop the dead assignments in `__init__`, `is_on`, `handle_attribute_updated` and `async_update`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #821 +/- ##
==========================================
- Coverage 97.29% 97.27% -0.02%
==========================================
Files 55 55
Lines 10933 10930 -3
==========================================
- Hits 10637 10632 -5
- Misses 296 298 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Apparently this (unintentionally) tested failing entity creation 😅: https://app.codecov.io/gh/zigpy/zha/pull/821/indirect-changes. Whatever entity that affects is now correctly filtered out using But maybe we should still have a basic test for the broad except. |
There was a problem hiding this comment.
Pull request overview
This PR removes the unused BinarySensor._state attribute and its associated write sites, relying solely on the cluster attribute cache for binary sensor state computation.
Changes:
- Drop
_stateinitialization inBinarySensor.__init__. - Remove dead
_statewrites inis_on,handle_attribute_updated, andasync_update. - Keep state-change emission based on recomputed
state(viais_on) rather than a stored field.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def is_on(self) -> bool: | ||
| """Return True if the switch is on based on the state machine.""" | ||
| self._state = raw_state = self._cluster.get(self._attribute_name) | ||
| raw_state = self._cluster.get(self._attribute_name) |
There was a problem hiding this comment.
Well, that's unrelated to this PR but a good thing to fix.
`discover_entities_for_endpoint` wraps each entity instantiation in a broad `except` so one broken entity is skipped (and logged) rather than aborting discovery for the whole endpoint. This path was previously only exercised incidentally; #821 removed the `BinarySensor._state = self.is_on` init call that used to trip it, leaving discovery.py:355-357 uncovered. Add an explicit test: patch `Switch.__init__` to raise, join the device, and assert the failure is logged and the switch entity skipped while the device still joins.
Addresses comment in:
What
BinarySensor._stateis only ever assigned, never read, so this drops the four dead write sites (__init__,is_on,handle_attribute_updated,async_update).Why
The state of a binary sensor is always recomputed directly from the cluster:
stateproperty returnsself.is_on,is_onreadsself._cluster.get(self._attribute_name), andmaybe_emit_state_changed_eventcomparesself.state(→is_on).Nothing reads
self._state— not the module, not the basePlatformEntity, not the tests. (Other platforms such asswitch,light,lockandcovergenuinely use their own_state; this is specific tobinary_sensor.)Removing the attribute also means
__init__no longer callsis_on, so the init-time guard added in #803 (if not self.is_supported(): self._state = False, to stopis_onraising while importing legacy diagnostics) is no longer needed — this can replace that hunk. See #803 (comment).