-
-
Notifications
You must be signed in to change notification settings - Fork 34.1k
Removed rounding of durations in Here Travel Time sensors #146838
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
Removed rounding of durations in Here Travel Time sensors #146838
Conversation
Hey there @eifinger, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
Looks good from my POV |
duration=duration / 60, | ||
duration_in_traffic=duration_in_traffic / 60, | ||
distance=distance, |
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.
where is this used? I think we can remove the / 60
as well. Sensors have a native unit of measurement, which is the unit the entity returns the state in, and a suggested unit of measurement. So if we get the value in hours, we can set the native unit of measurement in hours and set the suggested unit of measurement to minutes and that would save a bit of extra calculation
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.
It is used in the duration and duration_in_traffic sensors:
core/homeassistant/components/here_travel_time/sensor.py
Lines 53 to 66 in 61b0089
SensorEntityDescription( | |
translation_key="duration", | |
icon=ICONS.get(travel_mode, ICON_CAR), | |
key=ATTR_DURATION, | |
state_class=SensorStateClass.MEASUREMENT, | |
native_unit_of_measurement=UnitOfTime.MINUTES, | |
), | |
SensorEntityDescription( | |
translation_key="duration_in_traffic", | |
icon=ICONS.get(travel_mode, ICON_CAR), | |
key=ATTR_DURATION_IN_TRAFFIC, | |
state_class=SensorStateClass.MEASUREMENT, | |
native_unit_of_measurement=UnitOfTime.MINUTES, | |
), |
They are currently configured with minutes.
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.
Right, but what is the source unit then?
We should add a device class for these 2 (I think I saw a PR of that fly by last weekend)
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.
If we remove the rounding the source unit is seconds.
The PR for the device_class is: #146804
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.
Right, then we can just set the native_unit_of_measurement to seconds and set a suggested_unit_of_measurement to minutes and then we don't have any conversion logic in the integration anymore
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.
You're right, I opened PR #146804 to add the device class, if you need maybe we can merge it first.
And about the native unit of measurement, I was also thinking of making it seconds, however it makes HA show "783 s" instead of "13min 3s".
It seems that this behavior is linked to the duration value being reported as a float by the library, even though the decimal part is always 0.
Either we could change the duration type returned by the library, or we could add an int conversion for duration sensors in this integration.
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.
Home Assistant will do that conversion for you with suggested_unit_of_measurement :)
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.
Nevermind, the conversion is in the integration itself, I'll send a commit to fix that
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.
Please rebase
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>
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.
Tests are failing
Sorry for all these requests, everything should be good now 👍 |
Proposed change
Previously, time sensor values were rounded off, reducing their accuracy.
Having untouched float values allows automation to have the arrival value to the second, and the history graph to be more precise.
Finally, this doesn't cause any inconvenience, as the UI only displays one digit after the decimal point, and for automations there's the
states(entity_id, rounded=True)
template.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: