Add Priority Status sensor to Lyric integration - #177065
Conversation
Resideo's priority endpoint reports a top-level hold status (e.g. "NoHold") alongside the room priority data already used by the Room Priority select entity. This was parsed but never surfaced. Adds a dedicated diagnostic sensor reading from priorities_dict, gated the same way as the existing select entity (thermostat device with populated room data).
|
Hey there @timmo001, 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
Adds a diagnostic Lyric sensor exposing the thermostat’s room-priority hold status.
Changes:
- Creates the sensor for thermostats with room data.
- Reads status from
priorities_dict. - Adds the entity translation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
homeassistant/components/lyric/sensor.py |
Implements and registers the priority-status sensor. |
homeassistant/components/lyric/strings.json |
Adds the sensor name. |
| priority = self.coordinator.data.priorities_dict.get(self._mac_id) | ||
| if priority is None: | ||
| return None | ||
| return priority.status |
| _attr_entity_category = EntityCategory.DIAGNOSTIC | ||
| _attr_translation_key = "priority_status" |
There was a problem hiding this comment.
Lyric binary sensor and new sensors docs
#46986
| if accessory_sensor.suitable_fn(room, accessory) | ||
| ) | ||
|
|
||
| async_add_entities( |
abmantis
left a comment
abmantis
left a comment
There was a problem hiding this comment.
Please keep this as draft until the other PR fixing the coordinator is merged.
We don't want to add a new sensor that is always unavailable.
| if accessory_sensor.suitable_fn(room, accessory) | ||
| ) | ||
|
|
||
| async_add_entities( |
There was a problem hiding this comment.
although this integration was already doing it, usually async_add_entities is called once, with the full list of entities to add.
I think it is ok to keep it, but would be nice to change it later for consistency
There was a problem hiding this comment.
Agreed — this follows the pattern already established in the file (two separate async_add_entities calls existed before this PR for DEVICE_SENSORS/ACCESSORY_SENSORS). Happy to consolidate all three into a single call in a small follow-up cleanup PR if that's useful, rather than expanding this one's diff. Let me know if you'd rather I just do it here.
| _attr_entity_category = EntityCategory.DIAGNOSTIC | ||
| _attr_translation_key = "priority_status" |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
- Port the shared conftest.py fixtures from the sibling binary-sensor branch (real config-entry setup, aiolyric client patched at the Lyric.get_locations/get_thermostat_rooms boundary rather than mocked HTTP), since this branch predates that infrastructure. - Add an xfail(strict=True) end-to-end test documenting that the entity isn't created at all today: its creation gate reads rooms_dict, which never populates under the currently-pinned aiolyric (same currentPriority/priority key bug as the room_motion entity), and even if it did, LyricPriority.status reads the wrong JSON key too. Both are fixed in the same upstream aiolyric#165 diff. - Add direct unit tests for LyricPriorityStatusSensor.native_value (present/missing priority data), which don't depend on that fix and give real, non-xfail coverage of the new entity class and the async_add_entities gating logic - closing the codecov/patch gap Copilot flagged (new code in an already well-covered sensor.py). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| @pytest.mark.xfail( | ||
| strict=True, | ||
| reason=( |
…ntirely Same fixture/patching cleanup already applied to the sibling branches, plus the bigger change joostlek's "let's not add failing tests directly" pushed toward: replace the xfail end-to-end test and the two direct LyricPriorityStatusSensor.native_value unit tests with a single real snapshot_platform test. - fixtures/locations.json now has two devices: one with a priority entry (state "NoHold"), one with room data but no priority entry (state "unknown") - both branches of native_value get exercised through real entity state instead of direct property calls. - rooms_dict/priorities_dict are set directly with lightweight stand-ins rather than real LyricPriority objects, since LyricPriority.status/ current_priority's field-name bugs (aiolyric#165) are aiolyric's own test suite's concern, not this integration's - this fixture tests our gating/display logic assuming the data contract is met, independent of whether the currently-pinned library can produce it today. - Verified new code (the async_add_entities generator + the whole LyricPriorityStatusSensor class) has zero coverage gaps; the only remaining "missing" lines are pre-existing, unrelated to this PR.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
tests/components/lyric/conftest.py:81
- Condense this fixture docstring to the non-obvious upstream constraint. Most of it narrates the setup and coverage already visible below, making the fixture harder to maintain.
"""Mock the aiolyric client, backed by a real Location and directly-set priority data.
Patches Lyric where the integration imports it (autospec, like the
mealie client mock). Location/device data comes from a real
LyricLocation parsed from a live-shaped fixture, so field-name
tests/components/lyric/test_sensor.py:38
- Move these unused fixtures to a
@pytest.mark.usefixturesdecorator. Neither fixture value is consumed by the test, so keeping them as parameters obscures which inputs the assertions actually use.
setup_credentials: None,
mock_lyric_api: MagicMock,
…ract Resideo's own API docs document locationID as an Integer, and aiolyric's get_thermostat_rooms already types it int, but this fixture quoted it as a string - inherited from early in this session before checking the real docs. No behavior change (the value is only ever interpolated into a URL), but the fixture now matches reality.
- Condense mock_lyric_api's docstring to the non-obvious constraint (the aiolyric field-name bug and why the second device exists); drop the narration of setup mechanics already visible in the code below. - Apply setup_credentials/mock_lyric_api via usefixtures in test_sensor instead of injecting them as unused parameters, matching our own "don't inject unused fixture arguments" rule.
|
@clutch2sft since you marked this as ready, is the concern I raised in my previous review addressed? The other PR is still open. |
| if accessory_sensor.suitable_fn(room, accessory) | ||
| ) | ||
|
|
||
| async_add_entities( |
There was a problem hiding this comment.
In a follow up PR, can you group these calls?
There was a problem hiding this comment.
100% happy to do that.
conftest.py, fixtures/locations.json, and snapshots/test_sensor.ambr were all add/add conflicts against home-assistant#177067's now-merged identical- pattern files. This branch's versions are supersets (LyricPriority parsing, priority.json, a second Bedroom device with no priority entry) so kept them, minus the dead LOCATION_ID/DEVICE_ID constants. Snapshot regenerated to include both devices' schedule_status entities alongside this PR's priority_status ones.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
homeassistant/components/lyric/sensor.py:313
- Update to a released aiolyric version that parses the live status field before exposing this sensor. The PR description states that the currently pinned aiolyric 2.1.2 reads the wrong JSON key, so this line will publish an incorrect value until the manifest, generated requirements, and live-shaped fixture are updated together.
return priority.status
tests/components/lyric/conftest.py:68
- Shorten this fixture docstring to avoid narrating straightforward implementation details. The assignments and snapshots already document these branches, while this detailed explanation will become stale as the fixture evolves.
"""Mock the aiolyric client, backed by a real Location and a real LyricPriority.
priorities_dict holds a real LyricPriority parsed from priority.json,
exercising the same priorityStatus key aiolyric's own
get_thermostat_rooms() parses production responses into. rooms_dict
|
Apologies for the conflict churn — in trying to keep each of these as a "smallest possible PR," I didn't think through how much they'd step on each other once any one of them landed. Should've expected that upstreaming five PRs that all touch the same sensor.py/strings.json regions was going to mean repeated resolves every time one merges. This pass I merged latest dev into all four open ones together and confirmed none of them reconflict with each other before pushing, so hopefully this holds a bit better going forward. Thanks for staying on top of the reviews. |
Per Copilot's low-confidence review: the branch/state narration was already evident from the assignments and the snapshot, so keep only why priorities_dict is a real object and why rooms_dict isn't.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
homeassistant/components/lyric/sensor.py:313
- Bump
aiolyricto a release containing theLyricPriority.statusfield-name fix before exposing this value. The PR description states that the currently pinned dependency reads the wrong JSON key, so this sensor can report an incorrect value in production and theNoHoldsnapshot is not valid against the declared dependency.
return priority.status
Per the docstring-brevity standard.
| class LyricPriorityStatusSensor(LyricDeviceEntity, SensorEntity): | ||
| """Define a Honeywell Lyric room priority hold status sensor.""" | ||
|
|
||
| _attr_entity_category = EntityCategory.DIAGNOSTIC |
There was a problem hiding this comment.
Okay so what is the state of this entity?
There was a problem hiding this comment.
Good question to push on — turned out to be worth digging into. Resideo actually documents this: the priority endpoint's status field is a 5-value enum (NoHold/TemporaryHold/HoldUntil/PermanentHold/VacationHold), and it's the exact same vocabulary this file already carries as PRESET_NO_HOLD etc. for setpoint_status — just never declared as a finite set anywhere in this integration.
Declared it properly: SensorDeviceClass.ENUM with options mapped from those raw API strings to snake_case keys, plus state translations in strings.json. Good catch — this is a nicer entity for it.
Per joostlek's review: the raw priorityStatus values Resideo documents for the priority endpoint (NoHold/TemporaryHold/HoldUntil/PermanentHold/ VacationHold) are the same vocabulary this file already has as PRESET_NO_HOLD etc. in const.py, just not previously declared as a finite set. Adds SensorDeviceClass.ENUM + options mapping raw API strings to snake_case option keys, with state translations in strings.json. Snapshot regenerated: state is now the mapped option key (no_hold) instead of the raw API string (NoHold).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
homeassistant/components/lyric/sensor.py:220
- Gating entity creation on
coordinator.data.rooms_dict.get(device.mac_id)can prevent the priority status entity from ever being created for a thermostat (e.g., if rooms haven’t been loaded yet or are empty). SinceLyricPriorityStatusSensor.native_valuealready handles missing priority data, consider creating the entity for all thermostats and letting it report unknown/unavailable until data arrives; if you must gate, check key presence (is not None) rather than truthiness so empty room dicts don’t suppress entity creation.
async_add_entities(
LyricPriorityStatusSensor(coordinator, location, device)
for location in coordinator.data.locations
for device in location.devices
if device.device_class == "Thermostat"
and coordinator.data.rooms_dict.get(device.mac_id)
)
homeassistant/components/lyric/sensor.py:47
_attr_optionsis built from dict values, which is mutable and implicitly depends on insertion order. Consider defining a dedicated, immutable sequence (e.g., a tuple constant) for the options in the intended order and using that for_attr_options, to avoid accidental mutation and make ordering explicit.
PRIORITY_STATUS_OPTIONS = {
PRESET_NO_HOLD: "no_hold",
PRESET_TEMPORARY_HOLD: "temporary_hold",
PRESET_HOLD_UNTIL: "hold_until",
PRESET_PERMANENT_HOLD: "permanent_hold",
PRESET_VACATION_HOLD: "vacation_hold",
}
homeassistant/components/lyric/sensor.py:300
_attr_optionsis built from dict values, which is mutable and implicitly depends on insertion order. Consider defining a dedicated, immutable sequence (e.g., a tuple constant) for the options in the intended order and using that for_attr_options, to avoid accidental mutation and make ordering explicit.
_attr_options = list(PRIORITY_STATUS_OPTIONS.values())
homeassistant/components/lyric/sensor.py:323
native_valuereturnsNonefor anypriority.statusnot present inPRIORITY_STATUS_OPTIONS, which will surface asunknowneven if the backend is returning a valid-but-unmapped status. To make this robust, ensurePRIORITY_STATUS_OPTIONSkeys match the exact set of values exposed byLyricPriority.status(including any special cases like holiday/vacation naming), or normalizepriority.statusbefore lookup so valid states don’t silently degrade to unknown.
priority = self.coordinator.data.priorities_dict.get(self._mac_id)
if priority is None:
return None
return PRIORITY_STATUS_OPTIONS.get(priority.status)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tests/components/lyric/conftest.py:32
- Remove this implementation-narrating comment;
NO_PRIORITY_DATA_MAC_IDand the unknown-state snapshot already express the test case, so these lines add no non-obvious constraint.
# Second device: has room data but no priority data yet, exercising the
# defensive "no priority entry" branch of LyricPriorityStatusSensor.
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
Tests are failing, can you take a look?
Resideo's priority endpoint reports a top-level hold status (e.g. "NoHold") alongside the room priority data already used by the Room Priority select entity. This was parsed but never surfaced. Adds a dedicated diagnostic sensor reading from priorities_dict, gated the same way as the existing select entity (thermostat device with populated room data).
Proposed change
Resideo's priority endpoint reports a top-level hold status (e.g.
"NoHold")alongside the room priority data already used by the Room Priority select entity.
This was parsed but never surfaced. Adds a dedicated diagnostic sensor reading from
priorities_dict, gated the same way as the existing select entity (thermostatdevice with populated room data).
Dependency note: needs #177022 (coordinator fetching priority data) to populate
priorities_dictat all. The aiolyric field-name fix this originally needed(
LyricPriority.statusreading the wrong JSON key) is resolved —aiolyric2.1.2contains the fix and this branch is pinned to it.
Type of change
Additional information
LyricPriority.statusfix intimmo001/aiolyric#165— merged and released in aiolyric 2.1.2, which this PR already pins.Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
Lyric binary sensor and new sensors docs #46986
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: