Add sensor platform for AirPatrol#158726
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a sensor platform to the AirPatrol integration, introducing temperature and humidity sensor entities that expose room temperature and humidity data from AirPatrol climate devices.
Key changes:
- Adds sensor platform with temperature and humidity sensors
- Implements test fixtures to support platform-specific testing
- Updates test snapshots to reflect new test parametrization
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
homeassistant/components/airpatrol/sensor.py |
Implements new sensor platform with temperature and humidity entities using the coordinator pattern |
homeassistant/components/airpatrol/const.py |
Adds Platform.SENSOR to the list of supported platforms |
tests/components/airpatrol/conftest.py |
Adds load_platforms fixture to enable platform-specific test isolation |
tests/components/airpatrol/test_sensor.py |
Adds comprehensive sensor entity tests using snapshot testing with multiple data scenarios |
tests/components/airpatrol/test_climate.py |
Updates climate tests to use the new load_platforms parametrization for better test isolation |
tests/components/airpatrol/snapshots/test_sensor.ambr |
Snapshot data for sensor entity tests covering both data present and unavailable states |
tests/components/airpatrol/snapshots/test_climate.ambr |
Updated snapshot names to reflect new test parametrization |
| @property | ||
| def climate_data(self) -> dict[str, Any]: | ||
| """Return the climate data for this unit.""" | ||
| return self.device_data.get("climate") or {} | ||
|
|
||
| @property | ||
| def available(self) -> bool: | ||
| """Return if the sensor is available.""" | ||
| return super().available and bool(self.climate_data) |
There was a problem hiding this comment.
This already is in the base entity
There was a problem hiding this comment.
Not really, it's in the climate entity. Should we move it to the base entity instead?
There was a problem hiding this comment.
Oh, it's in both. We can remove it from the climate entity then, but lets do that in a later PR. We can use the entity.py one here now
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
joostlek
changed the title
| @property | ||
| def climate_data(self) -> dict[str, Any]: | ||
| """Return the climate data for this unit.""" | ||
| return self.device_data.get("climate") or {} | ||
|
|
||
| @property | ||
| def available(self) -> bool: | ||
| """Return if the sensor is available.""" | ||
| return super().available and bool(self.climate_data) |
There was a problem hiding this comment.
Oh, it's in both. We can remove it from the climate entity then, but lets do that in a later PR. We can use the entity.py one here now
| @pytest.mark.parametrize( | ||
| "climate_data", | ||
| [ | ||
| { | ||
| "ParametersData": { | ||
| "PumpPower": "on", | ||
| "PumpTemp": "22.000", | ||
| "PumpMode": "cool", | ||
| "FanSpeed": "max", | ||
| "Swing": "off", | ||
| }, | ||
| "RoomTemp": "22.5", | ||
| "RoomHumidity": "45", | ||
| }, | ||
| None, | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Okay, snapshot tests are great because they are easy to make and they test a lot. However, they are bad at testing change. So in this case I would just opt to create a second test where you test what you'd expect when the climate data is None. That test is definitely smaller and makes the intention of the test a lot clearer
| @property | ||
| def climate_data(self) -> dict[str, Any]: | ||
| """Return the climate data for this unit.""" | ||
| return self.device_data.get("climate") or {} |
There was a problem hiding this comment.
| @property | |
| def climate_data(self) -> dict[str, Any]: | |
| """Return the climate data for this unit.""" | |
| return self.device_data.get("climate") or {} | |
| @property | |
| def climate_data(self) -> dict[str, Any]: | |
| """Return the climate data for this unit.""" | |
| return self.device_data["climate"] |
| def available(self) -> bool: | ||
| """Return if entity is available.""" | ||
| return super().available and self._unit_id in self.coordinator.data | ||
| return super().available and bool(self.climate_data) |
There was a problem hiding this comment.
I'd change this to 2 in statements
There was a problem hiding this comment.
Like this?
super().available
and self._unit_id in self.coordinator.data
and "climate" in self.device_data
and bool(self.climate_data)There was a problem hiding this comment.
I need to have the bool one since I'm checking if the climate field is "None", not only if the key exists.
|
The parent PR is now merged, can you please make your docs PR point to the right repository? |
Thanks! All done and ready for this to be merged. |
Breaking change
Proposed change
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: