8000 Fix wind speed change in NWS by MatthewFlamm · Pull Request #37222 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix wind speed change in NWS #37222

8000
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

Merged
merged 1 commit into from
Jun 29, 2020

Conversation

MatthewFlamm
Copy link
Contributor

Proposed change

NWS changes reported units of wind speed from m/s to km/hr. See #37203 .

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:

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

Copy link
Member
@bdraco bdraco left a comment

Choose a reason for hiding this comment

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

nws changing the units in the api without some type of notification (https://www.weather.gov/documentation/services-web-api) is not great.

It would nice if the pypi package took care of all the conversions and presented the wind speed in a reliable unit so we didn't have to handle any changes in home assistant itself in case they make changes to the api again. Its probably hard to predict what they will change next though.

@MatthewFlamm
Copy link
Contributor Author

The ideal solution would be if NWS would publish a full specification of the data, then we could map any incoming unit to the one we need. Since this isn't the case:

I'm going to implement logic in the pypi library to supply the units being reported and the known units (for wind speed this would be km_hr-1 and m_s-1). This still would require some overhead on the home assistant side, but only a mapping from NWS unit to home assistant unit would be required, i.e. this current change would be a one line change if an additional unit is introduced. As it is now, if it reverts back to m/s it will be an issue again. This is not good.

I don't like the idea of converting units in the pypi library specifically for home assistant. The ideal state, IMO, is that it would be used by more than one downstream package, which would make it more robust and have more potential contributors. Also, it would require additional code burden for unit conversion or additional dependencies like pint.

@bdraco
Copy link
Member
bdraco commented Jun 29, 2020

I don't like the idea of converting units in the pypi library specifically for home assistant. The ideal state, IMO, is that it would be used by more than one downstream package, which would make it more robust and have more potential contributors. Also, it would require additional code burden for unit conversion or additional dependencies like pint.

The thought was to provide reliable units for anything consuming the pypi package regardless of
changes in the data source. It seems like any other consumer of the package wouldn't like the units being changed out from under them either.

@bdraco
Copy link
Member
bdraco commented Jun 29, 2020

Should we tag this for 0.112?

@MatthewFlamm
Copy link
Contributor Author

You make a good point and this is actually easier all around. The unit conversion burden is also not super high. I will use the current units as the provided ones.

It fixes a bug, so it seems like a good idea to get into a 0.112 release.

@bdraco bdraco added this to the 0.112.0 milestone Jun 29, 2020
@balloob balloob merged commit 11debb1 into home-assistant:dev Jun 29, 2020
balloob pushed a commit that referenced this pull request Jun 30, 2020
@MatthewFlamm MatthewFlamm mentioned this pull request Jul 1, 2020
20 tasks
hahn-th pushed a commit to hahn-th/core that referenced this pull request May 2, 2025
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.

Wind Speed Data From National Weather Service Integration - Change in Units
4 participants
0