core

core copied to clipboard

Breaking change

The unit system is no longer controlled by an options setting of the HERE Traveltime integration but uses the built in custom units configurable per entity. The distance sensor now uses kilometers as a default and must be manually configured if it was previously using the imperial system.

Proposed change

Use SensorDeviceClass.DISTANCE for the distance sensor and remove the unit OptionsFlow setting.

Equivalent to https://github.com/home-assistant/core/pull/79285

Type of change

• [ ] Dependency upgrade
• [ ] Bugfix (non-breaking change which fixes an issue)
• [ ] New integration (thank you!)
• [ ] New feature (which adds functionality to an existing integration)
• [ ] Deprecation (breaking change to happen in the future)
• [x] Breaking change (fix/feature causing existing functionality to break)
• [ ] Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• [x] The code change is tested and works locally.
• [x] Local tests pass. Your PR cannot be merged unless tests pass
• [x] There is no commented out code in this PR.
• [x] I have followed the development checklist
• [x] The code has been formatted using Black (black --fast homeassistant tests)
• [ ] Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• [ ] Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• [ ] The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• [ ] New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• [ ] For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• [ ] Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• [ ] No score or internal
• [ ] 🥈 Silver
• [ ] 🥇 Gold
• [ ] 🏆 Platinum

To help with the load of incoming pull requests:

• [ ] I have reviewed two other open pull requests in this repository.

Note: https://github.com/home-assistant/frontend/pull/13831 has been merged

Thank you @epenet took me a while to test locally with the newest frontend (20minute build 😮)

I think we should not merge this until after the beta cut, what will happen currently is that users who previously got miles will have to change the relevant entities from UI to override from km to miles.

For next release, I hope we have a better solution where distances in km - but not mm - will be automatically converted to miles and so on.

@emontnemery can you point me to a discussion where I can learn how this automatic conversion will work? Because the current conversion/unit_system is completely handled within the here_travel_time integration.

@eifinger I've been chatting with @epenet and some others about it, there's nothing more concrete. I'll try do a rough outline and share 👍

@emontnemery I'm also interested in how the automatic conversion will work. I maintain the Mazda integration, which will have the same problem for its odometer sensor entity.

Note for code-owners/maintainers:

• if a component does not currently do any conversion, then it is safe to add a device-class during beta so that users can choose the unit
• if the component already has a mechanism to check self.hass.config.units, then it is safer to keep the existing behavior for 2022.10, and wait until the better solution is finalised (I have opened an architecture discussion here: https://github.com/home-assistant/architecture/discussions/809)

can you point me to a discussion where I can learn how this automatic conversion will work

@eifinger there's now a discussion here https://github.com/home-assistant/architecture/discussions/809, although there's no agreement

Beta is cut, I marked this ready for review again

I have a created a PR to enable opt-in/opt-out on the temperature conversion: https://github.com/home-assistant/core/pull/80464 If it is approved, then I think it will provide a way forward for opt-in or opt-out for DISTANCE and other device classes.

@epenet thanks for the link. This would provide a non-breaking way forward for this integration. However I think we should harmonize how units are treated and configured and remove the extra way this integration goes by providing another way to define units via its OptionsFlow. Even if it is a breaking change.

I think once https://github.com/home-assistant/core/pull/80604 is merged we can do this removal. It should even be "non-breaking"...

It's hard to keep track, but #80604 has been closed in favor of #80638

I will wait for the changes to be merged and then fix the conflicts

#80638 has been merged

Waiting on #80892

@epenet I rebased after the last PR got merged. Am I right to assume that this is not a breaking change anymore, thanks to your work?

Thanks @epenet. Could you please explain the benefits of keeping the parameters if there is only one known value?

Thanks @epenet. Could you please explain the benefits of keeping the parameters if there is only one known value?

I think you could (maybe) still make tests based on the unit system.

Previously the tests parametrization (IMPERIAL_UNITS/METRIC_UNITS) was assigned to CONF_UNIT_SYSTEM, now it could be assigned to hass.config.units instead using (US_CUSTOMARY_SYSTEM/METRIC_SYSTEM).

This would test behavior of the Sensor device class. I don't think testing that this works correct should be done in an (arbitrary) integration.

@epenet what do you think? I can add the parameters again, I'm not strictly against it.

@epenet @emontnemery I am not sure the breaking change message is correct or necessary at all. Could you check that?