Skip to content
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

Add wind_speed and visibility units to weather components #11166

Closed
wants to merge 2 commits into from

Conversation

PhracturedBlue
Copy link
Contributor

Description:

Adds units for wind_speed and visibility to weather components. Adds new 'speed' unit to valid units. Add conversions from metric to imperial units for speed
This change was requested in: #11136

Checklist:

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@pvizeli
Copy link
Member

pvizeli commented Dec 21, 2017

@fabaff can you look at this?

@fabaff
Copy link
Member

fabaff commented Dec 29, 2017

I don't like the idea that we start to implement our own way to convert unit of measurements. This smells like reinventing the wheel. Instead we should start to use pint or a similar library to do it. It doesn't stop with wind speed there are a lot of other unit of measurements which we need to convert one day.

@PhracturedBlue
Copy link
Contributor Author

How do you want me to proceed? Using a new library to convert units seems like a large architectural change. I think that is beyond the scope of this feature, and I see no realistic way that I can guess how you'd like it architected. The Ecobee weather component is pretty broken right now and needs a fix, be it the simpler version (#11136) or this one..

@pvizeli
Copy link
Member

pvizeli commented Apr 17, 2018

Looks dead

@pvizeli pvizeli closed this Apr 17, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
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.

None yet

5 participants