Add method _should_include to EntityTriggerBase#169837
Conversation
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
This PR generalizes entity trigger aggregation by adding an overridable _should_include() hook to EntityTriggerBase, then uses it to let triggers ignore states that lack optional attributes during first/last evaluation. In Home Assistant, that mainly affects media player mute triggers and climate target-humidity triggers, with shared trigger test helpers expanded to cover filtered states.
Changes:
- Added
_should_include()to the shared trigger base and routedcheck_all_match()/count_matches()through it. - Simplified media player muted/unmuted triggers to use the shared aggregation hook.
- Introduced dedicated climate target-humidity trigger classes and expanded common trigger parametrization helpers/tests for excluded states.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
homeassistant/helpers/trigger.py |
Adds the shared _should_include() hook and applies it to aggregate trigger evaluation. |
homeassistant/components/media_player/trigger.py |
Removes duplicated aggregate logic and relies on the base helper hook. |
homeassistant/components/climate/trigger.py |
Replaces factory-generated humidity triggers with classes that exclude climates lacking humidity data. |
tests/components/common.py |
Extends shared trigger test helpers to model separately-filtered “other” targeted entities. |
tests/components/media_player/test_trigger.py |
Updates muted-trigger parametrization to use extra excluded states. |
tests/components/climate/test_trigger.py |
Opts humidity trigger test cases into the new attribute-required helper behavior. |
| # State for the *other* targeted entities (the ones not under direct test). | ||
| # Usually equal to `included_state`; differs when the test exercises a | ||
| # scenario where targeted-but-not-under-test entities sit in a state that | ||
| # the trigger's `_should_include` override filters out of the all/count |
There was a problem hiding this comment.
The last sentence has weird grammar.
| the all/count checks. Transitioning from a no-attrs state into a target | ||
| state fires only for the muted trigger (no-attrs has is_muted=False, so | ||
| the transition into muted=True flips is_muted, but a transition into | ||
| muted=False does not). |
There was a problem hiding this comment.
This surprises me. I think it would be more logical to exclude transitioning from a no-attrs state to a target state from firing the trigger.
There was a problem hiding this comment.
Yes, I agree. That behavior is not introduced by this PR though. Let's fix it in a follow-up.
| on the transition to a target (count=1). Pass True only when the | ||
| trigger's `is_valid_transition` actually permits transitions out of the | ||
| extra-excluded state into a target — typically true for numerical | ||
| triggers (None -> numeric is a valid change/threshold cross), false for | ||
| state-based triggers like media_player.unmuted where moving from | ||
| no-attrs to muted=False is the same is_muted value. |
There was a problem hiding this comment.
I'd expect that we don't trigger from an excluded state to a target state for numerical cross threshold triggers as we don't know the initial state location, ie on which side of the threshold we started.
There was a problem hiding this comment.
Yes, agreed. Let's fix that too.
|
I split out the docstrings and comments to #169869, I'll merge that first and then rebase this one. |
emontnemery
force-pushed
the
triggers_improve_first_last_checks
branch
from
363fdc9 to
2e5938b
Compare
| for other_entity_id in other_entity_ids: | ||
| set_or_remove_state(hass, other_entity_id, included_state) | ||
| set_or_remove_state(hass, other_entity_id, others_state) |
There was a problem hiding this comment.
Will improve this in a follow-up
Proposed change
Add method
_should_includetoEntityTriggerBasereplacing the hardcoded list of excluded states (unavailable,unknown) which is checked when trigger behavior isfirstorlastThis allows triggers to ignore additional states when checking if all states meet a certain criteria.
The method was introduced for the
media_playermute triggers, and after some discussion we concluded we want to expand it to other triggers which consume optional state attributes.In this PR, the media player triggers
media_player.mutedandmedia_plaer.unmuted, which introduced the method, as well asclimate.target_humidity_crossed_threshold, andclimate.target_humidity_crossed_thresholdare migrated.Also, test helpers are improved so the new functionality can be tested without having to write custom tests.
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: