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
Re-add missing Growatt TLX values #84040
Re-add missing Growatt TLX values #84040
Conversation
Hey there @indykoning, @JasperPlant, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
I'm a bit confused about the failing coverage report, is this a new feature that has been added to PRs? I'm not sure how I'm supposed to add tests for the comment that I added ...... Also, this integration does not yet have tests written for the core sensor or the sensor types. Is it possible to get this approved and merged without the tests in the interests of re-instating the missing functionality and I can then follow up with a new PR to add test coverage to the rest of the integration in a separate PR? |
d40ac2e
to
e3ffbe2
Compare
Answered my own question, turns out there were a load of files missing from coveragerc, I plan to add the test cases in the future but for now the files have been excluded from the coverage checks. |
If you don't want to close an issue, take care what verbs you write before the issue link in the PR description. GitHub will automatically close issues that are prefixed with fixes, closes, and similar verbs in a PR description. I updated the PR description to remove the auto close connection for the issue. |
# - will return 0 until a non-zero value is registered | ||
# 2 - System has been running fine but temporarily resets to 0 briefly at midnight: | ||
# - will return the previous value | ||
# 3 - HA is restarted during the midnight 'outage' - Not handled: |
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.
Maybe solve 3 by inheriting from RestoreSensor and restoring the last state when creating the entity?
class RestoreSensor(SensorEntity, RestoreEntity): |
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.
Thanks for the suggestion I'll look into how I can do this - I was wondering if there's a nice way of getting the previous value of an existing sensor, clearly there is!!
If it's ok with you, I'll look to implement this in a future PR rather than this one as I'd like to get this one merged as-is.
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.
Yes. 👍
65980da
to
58ee6f3
Compare
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.
Thanks!
Proposed change
This implements the majority of the missing statistics to solve 90% of the ticket and get people working again.
In the future the remaining 10% will be added as this is dependent on an upstream library change which will take longer to implement.
Type of change
Additional information
Checklist
black --fast 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
..coveragerc
.To help with the load of incoming pull requests: