Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Sun WEG integration #88272
Sun WEG integration #88272
Changes from all commits
df7b45e
7eb2712
6bb1f62
a8b14d1
91c97cc
caa511e
765f781
ee994b8
966bffc
9f88ca5
9d66142
4fb10b6
6211b7c
a6fb05c
43ba596
1c1549c
f25c0e3
59b1084
a236f9a
6e808e0
8fb697b
8301e6b
7c43bac
77cccc7
43e8660
9ac899a
c472427
ab820f9
ae5c7ab
5a50249
c840220
a2f2351
9721e48
dcaedfc
348e39d
bb9629a
0102989
d535699
a353dd0
c7bec8f
adbc989
3ccc78f
1ebb336
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why aren't we using a
DataUpdateCoordinator
instead? That's our recommended way of polling a single api for multiple entities.https://developers.home-assistant.io/docs/integration_fetching_data#coordinated-single-api-poll-for-data-for-all-entities
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.
Using
__dict__
to access attributes is really bad practice.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.
to be clear,
getattr
should be used to get an attribute by its nameThere 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.
not a requirement, just a suggestion/option - you could combine these using a walrus operator
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.
not a requirement, just an idea
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.
might be better to make these string values enums?
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.
The sensor description is a detail of the sensor platform. Either we should move this function to the sensor platform or the function should be refactored to be unaware of the sensor entity description and just work on a base entity description or some other abstraction.
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.
strange variable name. Is it not previous unit?
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.
Entity descriptions should not be modified. They are considered frozen.
Generally if the attribute needs to be calculated dynamically it doesn't belong in the entity description. Then it's better to set it inside the entity or pass it to the entity after calculating the attribute.
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.
The workarounds do not seem bullet proof, for example if Home Assistant is restarted when we get a wrong reading the accumulated sum will be permanently wrong.
Considering these complications, I don't think it's correct to use
SensorStateClass.TOTAL_INCREASING
, the state class should be set toSensorStateClass.TOTAL
instead forinverter_energy_today
andtotal_energy_today
.Another option would be to not set any state class at all for the
*_today
readings, there's not really any reason to accumulate statistics for both a grand total and a daily reading.More details here: https://developers.home-assistant.io/docs/core/entity/sensor#how-to-choose-state_class-and-last_reset
Please fix this in a follow-up.
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.
another candidate for a walrus operator
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.
The type annotation is incomplete. Please type the whole signature, including parameters, when adding type annotations.
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.
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 define constants that are only used within a single module within that module. Only shared constants should go in a const.py module.