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
Add energy usage sensor to A. O. Smith integration #105616
Add energy usage sensor to A. O. Smith integration #105616
Conversation
device_details_list = [ | ||
build_device_details(device) for device in status_coordinator.data.values() | ||
] |
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.
Hmm, I don't really feel the whole abstraction to have a separate object for device details. Can we maybe just pass on the status_coordinator.keys() to the energy coordinator and call it a day? The in the sensor platform you can just get the data from the status coordinator and pass that into the new 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.
The status coordinator data is just a dict[str, dict[str, Any]]
. If you don't mind, I'd prefer to keep it this way because I really think it makes the code cleaner to have a strongly typed dataclass rather than just passing a dict around everywhere. I do agree with the first part of your suggestion though, about passing the status coordinator keys to the energy coordinator, and I've made that change.
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
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.
My point was more, the status coordinator already has the data of the devices, so why construct a list of device info. For me that kinda feels like we're storing the stuff twice.
For real type safety you could look into adding types to the lib as return value
44e577b
to
835dd93
Compare
77ed066
to
5b31fe9
Compare
…5616) * Add energy usage sensor to A. O. Smith integration * Address review comments * Address review comment * Address review comment * Create device outside of the entity class * Address review comment * remove platinum
…5616) * Add energy usage sensor to A. O. Smith integration * Address review comments * Address review comment * Address review comment * Create device outside of the entity class * Address review comment * remove platinum
Proposed change
Adds a sensor entity for energy usage to the A. O. Smith integration.
I wanted the data for this entity to update on a slower interval than the other entities, so I created a separate data update coordinator class. This required also creating a separate base entity class and moving the common attributes (used for device info) into a parent base entity class.
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
..coveragerc
.To help with the load of incoming pull requests: