Fix calculation of attributes in statistics #128475
Conversation
|
Hey there @ThomDietrich, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
frenck
added
the
smash
|
Hey @gjohansson-ST just a few comments from my side. Please do not see them as a general pushback as there are a few good ideas in here! Thanks for you contribution :) |
|
Hey @gjohansson-ST, besides those random code changes, I'm having a hard time to understand what the improvement is.
What did you fix?
Maybe not necessary but also didn't hurt. Great if you were able to boost performance.
What does that mean? |
So the short story is that users had problems and issues were raised for helpers which had flopping state attributes. In the end we should have a consistent behavior between the helpers on how this works so the experience for the user is the same regardless if they use |
|
@gjohansson-ST the PR has conflicts |
gjohansson-ST
force-pushed
the
statistics-uom-load-from-db
branch
from
3e0ec28 to
087e8e3
Compare
| if ( | ||
| device_class in DEVICE_CLASS_UNITS | ||
| and unit not in DEVICE_CLASS_UNITS[device_class] | ||
| ): | ||
| return None |
There was a problem hiding this comment.
As you have diversified the cases returning None, did you consider that any of those cases should instead create a warning? I myself are not sure...
There was a problem hiding this comment.
Maybe. I'm a bit hesitant given the UoM is calculated so it might bring warnings/issues which is confusing and don't make sense to the user.
Let's consider in a follow-up in that case
|
|
||
| def _derive_unit_of_measurement(self, new_state: State) -> str | None: | ||
| def _calculate_state_attributes(self, new_state: State) -> None: |
There was a problem hiding this comment.
In light of this new function I suggest to rename the existing function _update_attributes(self). Those names clash currently. How about something like _update_additional_attributes(self) or _calculate_custom_attributes(self)
There was a problem hiding this comment.
We should update both _update_attributes and the attributes dict to make it clear it's about extra state attributes.
Maybe that can happen in a separate PR, to not pollute this PR?
There was a problem hiding this comment.
I agree, let's "clean" this in a follow-up PR.
I didn't even consider self.attributes nor the _update_attributes method
There was a problem hiding this comment.
Feels like an important change for clarity, but good on the extra PR
gjohansson-ST
force-pushed
the
statistics-uom-load-from-db
branch
from
087e8e3 to
2d3bad0
Compare
There was a problem hiding this comment.
Looks good to me, thanks @gjohansson-ST 👍
I think the remaining open comments can be addressed in follow-up PRs, @ThomDietrich ?
Proposed change
Fix attributes for
statisticsBesides calculating on each state change it also calculates on loading from recorder.
Checks that uom, device class and state class works together.
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.To help with the load of incoming pull requests: