provide Squeezebox player sensor for next alarm timestamp#155788
Conversation
home-assistant
Bot
added
cla-signed
has-tests
integration: squeezebox
new-feature
labels
|
Hey there @rajlaud, @pssc, @peteS-UK, 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 adds player-level alarm sensors to the Squeezebox integration, complementing the existing server-level sensors. The new functionality includes three binary sensors for alarm states (active, snoozed, upcoming) and one timestamp sensor for the next scheduled alarm time.
Key changes:
- Added player-based alarm sensors (binary sensors for alarm states and timestamp sensor for next alarm)
- Refactored test setup to use platform-specific fixtures reducing code duplication
- Introduced new sensor entity classes that follow the existing SqueezeboxEntity pattern
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| homeassistant/components/squeezebox/sensor.py | Added player alarm timestamp sensor entity class and setup logic |
| homeassistant/components/squeezebox/binary_sensor.py | Added player alarm binary sensor entities for active, snoozed, and upcoming states |
| homeassistant/components/squeezebox/const.py | Added constants for new player alarm sensor keys |
| homeassistant/components/squeezebox/strings.json | Added translation keys for the new alarm sensor entities |
| tests/components/squeezebox/test_sensor.py | Added test coverage for player alarm sensor and refactored test structure |
| tests/components/squeezebox/test_binary_sensor.py | Added comprehensive tests for player alarm binary sensors |
| tests/components/squeezebox/test_switch.py | Fixed incorrect docstring describing media_player instead of switch |
| tests/components/squeezebox/conftest.py | Added alarm-related mock attributes to test player fixture |
wollew
changed the title
wollew
force-pushed
the
player_sensor_next_alarm
branch
from
d114a80 to
5a62894
Compare
| @property | ||
| def native_value(self) -> StateType: | ||
| """Sensor value directly from player coordinator.""" | ||
| return cast( | ||
| StateType, | ||
| getattr(self.coordinator.player, self.entity_description.key, None), | ||
| ) |
There was a problem hiding this comment.
Instead of using getattr, I'd rather see the entity description being extended with a value_fn that return the value of the entity, as that would be typed, we can avoid the cast as well
There was a problem hiding this comment.
Done. I was surprised to find that StateType does not include datetime. So I changed the return value of native_value() to datetime, because currently the only sensor returns exactly that. To have a more general implementation this would have to be StateType | date | datetime | Decimal according to https://developers.home-assistant.io/docs/core/entity/sensor, is this correct?
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
This PR provides a player sensor with the time stamp of the next scheduled alarm for this player. It is a follow-up to #154491 which provides binary sensors for player alarms.
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: