Skip to content

Add more units to distance conversion util#40181

Merged
balloob merged 11 commits intohome-assistant:devfrom
springstan:add-more-units-to-distance-conversion
Nov 9, 2020
Merged

Add more units to distance conversion util#40181
balloob merged 11 commits intohome-assistant:devfrom
springstan:add-more-units-to-distance-conversion

Conversation

@springstan
Copy link
Copy Markdown
Member

@springstan springstan commented Sep 17, 2020

Breaking change

Proposed change

#40116 has to be merged before

Adds the following length units to distance conversion util:

  • LENGTH_CENTIMETERS
  • LENGTH_MILLIMETERS
  • LENGTH_INCHES
  • LENGTH_YARD

Make LENGTH_YARD available in proximity integration.

cc @MatthewFlamm

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)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • 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:

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.
  • 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:

Copy link
Copy Markdown
Contributor

@DaAwesomeP DaAwesomeP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Please add tests for the new conversion functions. You should be able to model them off of the existing conversion function tests.

Comment thread homeassistant/util/distance.py
Comment thread homeassistant/util/distance.py
Copy link
Copy Markdown
Contributor

@DaAwesomeP DaAwesomeP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I assume that you checked that there are no instances of cm, in, or yd anywhere else in HA.

@springstan
Copy link
Copy Markdown
Member Author

Also, I assume that you checked that there are no instances of cm, in, or yd anywhere else in HA.

I would recommend to do that in a separate PR.

@MatthewFlamm
Copy link
Copy Markdown
Contributor

Even though I asked if this should be done, previous PRs for adding units and conversions were not accepted if they weren't used in the code. See #35575

Can these conversions be used anywhere?

@springstan
Copy link
Copy Markdown
Member Author

Can these conversions be used anywhere?

Yeah I have found the proximity integration which could utilize all of these added units and conversions. I would just have to add the units to that integration and convert it here:

convert(dist_to_zone, LENGTH_METERS, self.unit_of_measurement), 1

@MatthewFlamm
Copy link
Copy Markdown
Contributor

Miles and yards I suppose make sense to add, but I don't see how millimeters, centimeters,n or inches would be used. I would imagine most entities use GPS for location which does not have sub-meter accuracy.

@springstan
Copy link
Copy Markdown
Member Author

You are probably right. Centimeters, millimeters and inches are just too precise for device trackers. Will remove them from the proximity integration.

@@ -34,19 +47,35 @@ def convert(value: float, unit_1: str, unit_2: str) -> float:

if unit_1 == LENGTH_MILES:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a future PR, instead of doing hundreds of if…elif statements, we should rewrite this like this:

TO_METERS = {
  LENGTH_METERS: lambda meters: meters,
  LENGTH_YARD: lambda yards: yards * 0.9144,
  LENGTH_FEET: lambda feet: feet * 0.3048,
  ...
}

meters = TO_METERS[unit_1](value)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #43107

@balloob
Copy link
Copy Markdown
Member

balloob commented Nov 9, 2020

Don't forget to update the docs for Proximity.

@balloob balloob merged commit 67b3552 into home-assistant:dev Nov 9, 2020
@springstan
Copy link
Copy Markdown
Member Author

Don't forget to update the docs for Proximity.

Updated the docs accordingly: home-assistant/home-assistant.io#15578

@springstan springstan deleted the add-more-units-to-distance-conversion branch November 9, 2020 10:56
@springstan springstan mentioned this pull request Nov 11, 2020
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants