core
core copied to clipboard
Add Tado add meter readings service
Proposed change
Add service Add meter readings
to the Tado integration. Adding meter readings to Tado helps the user to gain usefull energy insights like cost savings and increased accuracy on estimations.
Type of change
- [ ] Dependency upgrade
- [ ] Bugfix (non-breaking change which fixes an issue)
- [ ] New integration (thank you!)
- [x] New feature (which adds functionality to an existing integration)
- [ ] Deprecation (breaking change to happen in the future)
- [ ] Breaking change (fix/feature causing existing functionality to break)
- [ ] Code quality improvements to existing code or addition of tests
Additional information
None applicable.
Checklist
- [x] The code change is tested and works locally.
- [x] Local tests pass. Your PR cannot be merged unless tests pass
- [x] There is no commented out code in this PR.
- [x] I have followed the development checklist
- [x] I have followed the perfect PR recommendations
- [x] The code has been formatted using Ruff (
ruff format homeassistant tests
) - [ ] Tests have been added to verify that the new code works.
If user exposed functionality or configuration variables are added/changed:
- [ ] Documentation added/updated for www.home-assistant.io
If the code communicates with devices, web services, or third-party tools:
- [x] The manifest file has all fields filled out correctly.
Updated and included derived files by running:python3 -m script.hassfest
. - [x] New or updated dependencies have been added to
requirements_all.txt
.
Updated by runningpython3 -m script.gen_requirements_all
. - [x] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
- [x] Untested files have been added to
.coveragerc
.
To help with the load of incoming pull requests:
- [x] I have reviewed two other open pull requests in this repository.
Hey there @chiefdragon, @erwindouna, mind taking a look at this pull request as it has been labeled with an integration (tado
) you are listed as a code owner for? Thanks!
Code owner commands
Code owners of tado
can trigger bot actions by commenting:
-
@home-assistant close
Closes the pull request. -
@home-assistant rename Awesome new title
Renames the pull request. -
@home-assistant reopen
Reopen the pull request. -
@home-assistant unassign tado
Removes the current integration label and assignees on the pull request, add the integration domain after the command. -
@home-assistant add-label needs-more-information
Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request. -
@home-assistant remove-label needs-more-information
Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.
I like the idea of adding more insightful information! Why did you choose to go far a service only? Having this periodically updated, saves the hazzle of manually running the service.
Also, please add a fixture for the test cases.
Why did you choose to go far a service only? Having this periodically updated, saves the hazzle of manually running the service.
The Tado integration is not aware of the meter reading, as those come from another integration or are manual input. This service call is needed to collect the meter reading before sending it to Tado. A simple automation on an interval can be created to send the new meter reading.
please add a fixture for the test cases.
There is no response in the service call, what would you like me to add?
The Tado integration is not aware of the meter reading, as those come from another integration or are manual input. This service call is needed to collect the meter reading before sending it to Tado. A simple automation on an interval can be created to send the new meter reading.
Where do you obtain the meter information? Right now it's a manual entry. An ideal use case would be to utilize the options of the config flow to let the user select which sensors should be used to populate the meter reading for Tado. Is the meter reading of Tado both compatible with m3 and kWh?
There is no response in the service call, what would you like me to add?
True, this is indeed the setter, not the getter.
please add a fixture for the test cases
Hi @erwindouna When adding a meter reading, Tado is could respond with an error, I thought it best to validate the response and raise a ServiceValidationError
when needed.
I found three different responses; a success response, an invalid_meter_reading response when you add a meter reading below the previous one or a duplicated_meter_reading response when you add multiple readings on the same day.
I wanted to add test cases for each of these, but I'm not sure how. Can you help out?
add_readings_success.json add_readings_invalid_meter_reading.json add_readings_duplicated_meter_reading.json
You can patch the Tado library (PyTado, the upper laying library), and return fixtures with the desired results per call. Make three test cases. Use the side_effect to mimic the errors (invalid and duplicate). Check the Fastdotcom service test for an example. Use something like json.loads(load_fixture("your_fixture.json"))
as a return value, to let it run through the PyTado function for the meter readings.
@zweckj @erwindouna please review
@zweckj @erwindouna please review
don't ping people, reviewers get a notification anyways
Very nice progress thus far. @niro1987! We're getting close to completion. :)
Looks good to me. Let's await a Core Members review now. :)
await a Core Member
it needs an initial accepted review to get that kind of attention. @erwindouna You're a code-owner for this integration, you are able to provide that initial review.
@zweckj can you complete the requested review please? I'm hoping to get this done before 2024.02
@zweckj can you complete the requested review please? I'm hoping to get this done before
2024.02
I already told you: Don't ping people it's unpolite! I've already looked at it and it looks good to me, but I am no maintainer here, so my review doesn't have any point, except leaving (hopefully) better code when a maintainer finally looks at it (which can take weeks to months)
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks :+1:
1 review requesting changes by reviewers
All reviews are resolved
@niro1987 you included a lot of unrelated commits. Please rebase and open a new PR, closing this one to avoid spamming linked developers.
You have now quite a lot unneeded extra commits. Is there a reason you asked so many people for the review?
You have now quite a lot unneeded extra commits. Is there a reason you asked so many people for the review?
It is due to a wrong rebase, Bot automatically ask codeowners for changed files.
https://github.com/home-assistant/core/pull/111552