Add geocaching cache sensors #145453
Add geocaching cache sensors #145453
Conversation
|
Hey there @Sholofly, @reinder83, 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.
Let's make the PRs even smaller. I think it's also important to keep in mind that we don't necessarily look at the lines of code, but rather how much it's touching, and this is
- introducing a base entity
- Adding a new platform
- Adding more entities
Which is too much for one PR
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
@joostlek This PR only introduces a new sensor, a cache sensor. The only thing I could reasonably remove would be the location entity, but then I would get the comment "why is there a base class? it is only derived by a single class". I could remove some of the cache entities, but those are pretty much a few copy pasted lines with slightly different logic (line 82-112 in |
Yes and no, you add more entities, but you are adding the device tracker in a different way.
I mean I do see that there is an ongoing effort, so you won't hear this from me. |
|
@joostlek So if I remove the device tracker from this PR (and potentially revert the |
|
I think that PR would be a reviewable PR yes. You can add the others as well, but let's not open more than 3 at the same time (and make sure they don't depend on each other) if you want to work more in parallel |
|
@joostlek Reverted |
There was a problem hiding this comment.
Okay so does the current coordinator already fetch data for the caches?
If so, let's create a base base entity that can be used for the profile and for the caches. This will inherit the coordinatorEntity and declare _attr_has_entity_name.
We can do with a lot less functions, we can inline get_cache_device_info and get_cache_entities
Yes, when the
I removed the
|
| GeocachingCacheSensorDescription( | ||
| key="found_date", | ||
| device_class=SensorDeviceClass.DATE, | ||
| value_fn=lambda cache: cache.found_date_time, | ||
| ), |
There was a problem hiding this comment.
When the user finds this specific cache
There was a problem hiding this comment.
which user? the signed in one or someone else? Like, I am not sure what feature this would add
There was a problem hiding this comment.
The one who authenticates with Geocaching. So me, if I install it. For every cache I add to track, I would see the date when I found that cache. So I can track which caches I have found (easier with the binary sensor) or track history of how often I find caches etc
| GeocachingCacheSensorDescription( | ||
| key="favorite_points", | ||
| native_unit_of_measurement="points", | ||
| value_fn=lambda cache: cache.favorite_points, | ||
| ), |
There was a problem hiding this comment.
what's this exactly? Can we add the unit of measurements to strings.json?
There was a problem hiding this comment.
The number of times the cache has been favorited. I used the same code for this as the ProfileSensor that already existed before I started by work, favorite_points.
I guess we could, but none of the existing entities does this so I just did the same to keep it cohesive throughout the component.
There was a problem hiding this comment.
I don't mind adding it in any case, it's a quick win :)
There was a problem hiding this comment.
I migrated them all in 2720b28 as I do not like the idea of having different solutions to the same problem in the same component. I want either all to use strings.json, or all to use native_unit_of_measurement.
|
|
||
| # Base class for a cache entity. | ||
| # Sets the device, ID and translation settings to correctly group the entity to the correct cache device and give it the correct name. | ||
| class GeoEntityBaseCache(GeocachingCacheEntity, SensorEntity): |
There was a problem hiding this comment.
This one can be merged with GeoEntityCacheSensorEntity now
There was a problem hiding this comment.
Merged how? Moving the entity_description and native_value in here?
I thought you said I should keep this as a good base class, since I will need to derive from this class later when adding the tracker platform. The tracker did not derive from this class previously because it did not use a GeocachingCacheSensorDescription. Are you suggesting I rewrite the tracker to use a sensor description? Previously it derived from TrackerEntity and not SensorEntity as this does, but I am not sure of the inheritence of these classes so TrackerEntity might be a subclass of SensorEntity in which case the double deriving should be fine.
Probably just a misunderstanding, I don't think I understand what you mean with this comment.
| for cache in status.tracked_caches: | ||
| entities.extend( | ||
| [ | ||
| GeoEntityCacheSensorEntity(coordinator, cache, description) | ||
| for description in CACHE_SENSORS | ||
| ] | ||
| ) |
There was a problem hiding this comment.
you can add the outer for loop to the list comprehension
There was a problem hiding this comment.
Sure, but I thought the readability of nested list comprehensions would be worse. I changed it anyway in eb18e79, but I definately prefer the old way for clearer code to avoid misunderstandings when there are so many maintainers.
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
This was awaiting our review. |
|
I think it would be great to add tests of the sensors in a follow-up PR 👍 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds new sensor entities for individual geocaching caches to the geocaching integration. It extends the existing profile-level sensors (finds, hides, etc.) with cache-specific sensors that track information like found date, favorite points, and hidden date for individual caches.
Key changes:
- Creates cache-specific sensor entities with their own device groupings
- Adds base entity classes to support both profile and cache entities
- Updates string translations and icons for the new cache sensors
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| homeassistant/components/geocaching/sensor.py | Restructures sensor setup to support both profile and cache sensors, adds new cache sensor classes |
| homeassistant/components/geocaching/entity.py | New file defining base entity classes for profile and cache entities with proper device grouping |
| homeassistant/components/geocaching/strings.json | Adds translations and units for new cache sensors, restructures existing sensor definitions |
| homeassistant/components/geocaching/icons.json | Adds icon definitions for new cache sensor types |
Comments suppressed due to low confidence (5)
homeassistant/components/geocaching/sensor.py:1
- The
native_unit_of_measurementfields are being removed from the entity descriptions but the corresponding units instrings.jsonuse different strings ('caches' vs 'cache'). This inconsistency could cause confusion - ensure the unit naming is standardized between the code and translation files.
"""Platform for sensor integration."""
homeassistant/components/geocaching/sensor.py:1
- The
native_unit_of_measurementfields are being removed from the entity descriptions but the corresponding units instrings.jsonuse different strings ('caches' vs 'cache'). This inconsistency could cause confusion - ensure the unit naming is standardized between the code and translation files.
"""Platform for sensor integration."""
homeassistant/components/geocaching/sensor.py:1
- The
native_unit_of_measurementfields are being removed from the entity descriptions but the corresponding units instrings.jsonuse different strings ('caches' vs 'cache'). This inconsistency could cause confusion - ensure the unit naming is standardized between the code and translation files.
"""Platform for sensor integration."""
homeassistant/components/geocaching/sensor.py:1
- The
native_unit_of_measurementfields are being removed from the entity descriptions but the corresponding units instrings.jsonuse different strings ('caches' vs 'cache'). This inconsistency could cause confusion - ensure the unit naming is standardized between the code and translation files.
"""Platform for sensor integration."""
homeassistant/components/geocaching/sensor.py:1
- The
native_unit_of_measurementfields are being removed from the entity descriptions but the corresponding units instrings.jsonuse different strings ('caches' vs 'cache'). This inconsistency could cause confusion - ensure the unit naming is standardized between the code and translation files.
"""Platform for sensor integration."""
| self._attr_translation_key = f"cache_{key}" | ||
|
|
||
|
|
||
| class GeoEntityCacheSensorEntity(GeoEntityBaseCache, SensorEntity): |
There was a problem hiding this comment.
This class inherits from both GeoEntityBaseCache (which already inherits from SensorEntity) and SensorEntity directly, creating redundant inheritance. Remove the direct SensorEntity inheritance since it's already provided by the base class.
| class GeoEntityCacheSensorEntity(GeoEntityBaseCache, SensorEntity): | |
| class GeoEntityCacheSensorEntity(GeoEntityBaseCache): |
4 participants
Breaking change
Proposed change
Adds new sensors for geocaching caches. An upcoming PR will add them to the configuration flow, so you can track caches.
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: