Skip to content

Add speed conversion function#53846

Merged
emontnemery merged 16 commits intohome-assistant:devfrom
rianadon:speed-conversion
Nov 9, 2021
Merged

Add speed conversion function#53846
emontnemery merged 16 commits intohome-assistant:devfrom
rianadon:speed-conversion

Conversation

@rianadon
Copy link
Copy Markdown
Contributor

@rianadon rianadon commented Aug 2, 2021

Proposed change

Similar to how the distance and temperature units have conversion functions, I created one for converting between speeds, along with tests.

While this refactor does not result in any immediate gains, this function will help implement #48641, which requires converting speeds between user-configurable units.

I've also edited existing kph <--> mph unit conversions using the length conversion function to use the speed conversion function.

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

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.

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @DylanGore, mind taking a look at this pull request as it has been labeled with an integration (met_eireann) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@probot-home-assistant
Copy link
Copy Markdown

Hey there @raman325, mind taking a look at this pull request as it has been labeled with an integration (climacell) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@probot-home-assistant
Copy link
Copy Markdown

Hey there @Danielhiversen, @thimic, mind taking a look at this pull request as it has been labeled with an integration (met) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@probot-home-assistant
Copy link
Copy Markdown

Hey there @MatthewFlamm, mind taking a look at this pull request as it has been labeled with an integration (nws) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@rianadon
Copy link
Copy Markdown
Contributor Author

rianadon commented Aug 2, 2021

I apologize for the excessive bot pinging, probably due to refactoring across a few different components. I did not expect that to happen.

@rianadon rianadon changed the title Add speed conversion function Add speed conversion function & add speed to units system Aug 2, 2021
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 1, 2021

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 1, 2021
@github-actions github-actions bot closed this Sep 12, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Sep 13, 2021
@emontnemery emontnemery reopened this Sep 13, 2021
@rianadon rianadon requested review from emontnemery and removed request for a team October 23, 2021 21:26
@emontnemery
Copy link
Copy Markdown
Contributor

emontnemery commented Oct 25, 2021

This looks mostly good.

One concern though; let's move the adding of a default speed unit for the imperial and metric unit systems to a separate PR:

  • SI unit for speed is m/s not km/h, although km/h is probably more commonly used
  • The unit traditionally used for wind speed and rainfall may differ from the unit used for other speeds
    • We certainly don't want rainfall in mm/day to be converted to km/h

This can be discussed in a separate PR, then this PR is ready for merge IMO.

@rianadon rianadon mentioned this pull request Oct 25, 2021
14 tasks
@home-assistant home-assistant unlocked this conversation Oct 29, 2021
@github-actions github-actions bot removed the stale label Oct 29, 2021
@rianadon
Copy link
Copy Markdown
Contributor Author

rianadon commented Nov 3, 2021

@emontnemery pinging you in case you didn't get notified of my new commits.

I parameterized the tests slightly differently than you suggested (by using only one parameterize call and passing in the expected conversion results). Let me know if you still like your method better.

@emontnemery
Copy link
Copy Markdown
Contributor

It's OK, I think.
By parametrizing to and from units separately all permutations are tested, but maybe that was a bit over the top.

Just a couple of small comments on the tests.

@rianadon
Copy link
Copy Markdown
Contributor Author

rianadon commented Nov 5, 2021

I think all of the conversion factors get tested even if there's not every single permutation, since I use every unit at least once.

I don't see your other comments on the tests. Did I accidentally resolve them?

@emontnemery
Copy link
Copy Markdown
Contributor

I don't see your other comments on the tests. Did I accidentally resolve them?

I forgot to "publish" the review, sorry about that 🤦

@rianadon
Copy link
Copy Markdown
Contributor Author

rianadon commented Nov 9, 2021

No worries! Thank you for the review. I've addressed the comments.

Copy link
Copy Markdown
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Thanks, @rianadon 👍

@emontnemery emontnemery merged commit a102c42 into home-assistant:dev Nov 9, 2021
@emontnemery emontnemery changed the title Add speed conversion function & add speed to units system Add speed conversion function Nov 9, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants