-
-
Notifications
You must be signed in to change notification settings - Fork 36.6k
Geocaching integration feature updates #134017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Nearby caches
Add trackable entities, and some additional cache fields
removed comments
Conflig flow
…n, and connect the user config with HA backend
Multi code configuration for trackables, and connect configuration with backend
Add nearby cache count and radius to configuration
Replace cache find count entity with found date and found switch
Add trackable location entity and journey log attributes
joostlek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please split the PR up into smaller parts and try to keep PRs scoped, as in, not bigger than needed.
So I would suggest first opening a PR to bump the library and then go from there
|
@joostlek The new library version is connected to the changes in this PR. They were developed alongside each other, bumping the library version without updating the integration code will not do anything as the old code does not use the new features of the library. I have not tested using the old code with the new library so it might even cause issues. How do you propose we split the PR? I cannot think of any reasonable splits myself, since the new version is tested as a whole and not in small parts. |
But that is fine, that means that when that is one PR, that PR has everything it needs to support the new version. This PR is too big to review. The bigger they are, the slower they get in. You can add the base entity in a separate PR, do the other platform improvements in separate PRs, add new platforms in a separate PR. But I haven't looked in the code in depth as it does too much. |
| "domain": "geocaching", | ||
| "name": "Geocaching", | ||
| "codeowners": ["@Sholofly", "@reinder83"], | ||
| "codeowners": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the codeowners need to be changed, not all contributers have to be owner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Home Assistant automatically changed that file, but we can remove it if you would like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that's why, it's fine by me, I dont know what is commonly done
reinder83
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks good to me, very cool new additions, but I cannot review the integration part, maybe someone with more experience in this could review this.
The only thing I wonder is if we should add more codeowners?
|
I mean if there are people who want to step up and help with the integration and issues, they are free to do so. We don't really have any process in place with stale codeowners or too many codeowners |
|
@joostlek I understand where you are coming from, but splitting it would mean we need to test every part all over again to ensure we don't break anything. The PR only really has two parts:
Most of the code is just adding the new sensors, with abstractions to be able to reuse the code for the sensors. So should we do a PR with just the entity abstractions first? The sensors themselves are small, they just leverage the abstractions. I think it would be pretty confusing adding a bunch of structure for just a single sensor or so. But if that confusion is worth it to have a smaller PR then I guess it would be possible. But again, I would need to retest everything again to make sure it does not break. |
|
This PR can be split up in at least 5 or 6 PRs. The thing is, if you didn't change existing code or config, it should be fine. The downside with big PRs is, if there is something wrong in the base of that PR, the rest of the PR needs to adjusted too. If the PR is smaller, that is easier as you don't have to move that much code around. And I don't mind to give directions on how to best get it in, and I am sure that others are also willing to help testing, but we should adhere to the guidelines we set to the project. If every integration gets one of those PRs every 6 months I would scream. |
|
Hi @marc7s, It looks like this PR is not coming through due to the size. I probably can help testing if you split it up. Tell me what I can do! |
I have now spent a few days separating the code into several PRs, which I will start opening now. The first is #139878 I have tested each PR somewhat, but since I know the code worked as a whole there may be some issues in a specific phase that gets fixed by a later PR. But I tried configuring the Geocaching integration for each PR, and it seemed to work for all PRs I removed the nearby caches feature from the split up phases, as the implementation was not 100% with the entities not being dynamically generated. FYI @joostlek |
|
Alright, since the splitting has started, I'll go ahead and close this one up to keep our queues clean. Please make sure not to stack up the PRs, as that just causes noise. I've closed the others for that reason (let's re-open and rebase them when they are relevant again). ../Frenck |
Proposed change
This update adds several new features to the Geocaching integration, enabled by the release of version 0.3.0 of the
geocachingapipackage, changelog. It adds support for Geocaches and Trackables, creating a device for each cache or trackable containing the relevant entities for each item. Here is an example of a device generated for a Trackable, with entities related to that device:We have provided a README for the integration, containing some example templates for presenting caches and trackables on a dashboard.
Type of change
Additional information
reverse_geocode. We are not sure how this is handled in HA, but we did not add that dependency to therequirementsin the manifest. We recloned the repository and rebuilt the dev container and it still worked, so we assume dependencies of requirements are installed automaticallyChecklist
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: