Added binary sensor for battery status to the PTDevices integration#169862
Added binary sensor for battery status to the PTDevices integration#169862frogman85978 wants to merge 6 commits intohome-assistant:devfrom
Conversation
home-assistant
Bot
added
cla-signed
has-tests
integration: ptdevices
new-feature
new-platform
labels
|
Hey there @ParemTech-Inc, 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 extends the ptdevices integration by adding a new battery-status binary sensor entity and introducing snapshot-based test coverage for the binary sensor platform.
Changes:
- Added a
binary_sensorplatform that exposes a low-battery binary sensor based on the device’s reportedbattery_status. - Registered
Platform.BINARY_SENSORfor the integration and added translation strings for the new entity. - Added snapshot tests + snapshots for PTDevices binary sensors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
homeassistant/components/ptdevices/__init__.py |
Registers the new binary sensor platform for setup. |
homeassistant/components/ptdevices/binary_sensor.py |
Implements the battery-status binary sensor entity and setup logic. |
homeassistant/components/ptdevices/strings.json |
Adds entity translation string for the new binary sensor. |
tests/components/ptdevices/test_binary_sensor.py |
Adds snapshot test covering binary sensor entities. |
tests/components/ptdevices/snapshots/test_binary_sensor.ambr |
Stores the expected entity-registry + state snapshots for the new binary sensor. |
test_binary_sensor.py
| class PTDevicesBinarySensors(StrEnum): | ||
| """Store keys for PTDevices binary sensors.""" | ||
|
|
||
| DEVICE_BATTERY_STATUS = "battery_status" |
There was a problem hiding this comment.
Do you expect this to be extended later on? Else, I don't think a StrEnum is not needed here.
There was a problem hiding this comment.
Yes. This will be extended in future updates.
| class PTDevicesBinarySensorEntityDescription(BinarySensorEntityDescription): | ||
| """Description for PTDevices binary sensor entities.""" | ||
|
|
||
| is_on_fn: Callable[[dict[str, str | int | float | None]], bool] |
There was a problem hiding this comment.
That's a lot of types in the dict. Are you mimicing the StateType here? If so, you can put it in directly.
| """Setup PTDevices binary sensors based on config entry.""" | ||
| coordinator = config_entry.runtime_data | ||
|
|
||
| known_sensors: set[tuple[str, str]] = set() |
There was a problem hiding this comment.
Since you're growing with sensors, or better devices, I think the coordinator should be responsible of adding or removing devices and not per platform. Check the Portainer integration for an example, but I think that way we can reduce potential duplicate code when adding new platforms. :)
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
This PR adds a binary sensor for the battery status, (either Normal or Low). Along with snapshot testing for the binary sensors. No changes to dependencies are required.
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: