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 NWS weather #23647

Merged
merged 38 commits into from Aug 23, 2019

Conversation

@MatthewFlamm
Copy link
Contributor

commented May 3, 2019

Description:

Adds NWS weather platform. NWS weather.

There are two forecast modes supported by NWS, one is a separate day and night forecast (two forecasts a day) or hourly. Hourly works well with the standard card. Day and night displays the hour of the forecast which is confusing. This platform has an additional forecast attribute to tag which forecast is day or night. A precipitation probability is also exposed. A modified weather card PR in the frontend is planned to utilize these features to better support day and night forecasts.

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#9372

Example entry for configuration.yaml (if applicable):

weather:
  - platform: nws
    api_key: my_email

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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 (example).
  • New dependencies have been added to requirements in the manifest (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
Copy link
Member

left a comment

Just some comments to get the review started.

homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved
homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved
homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved
homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved
homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved
homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved
homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved
homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved
homeassistant/components/nws/weather.py Outdated Show resolved Hide resolved
@MatthewFlamm

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

I'm going to remove the TODO from the PR. My frontend dev environment is totally hosed, and I'm not sure how to fix it. My frontend changes did work several versions ago, but I can't test that it works today. I don't want to bog down this PR with these issues.

@MatthewFlamm MatthewFlamm referenced this pull request May 15, 2019
7 of 9 tasks complete
@MatthewFlamm

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

#23875 is a similar PR for a NWS/NOAA sensor platform, which uses the same data source and library. These should be considered together, as we have a few inconsistencies across the two implementations. I'm proposing that we work together to resolve the inconsistencies, unless there is a preference to implement the weather platform or the sensor platform first. Then we can resolve the differences during the second PR.

@MatthewFlamm MatthewFlamm force-pushed the MatthewFlamm:add_nws branch from 5a93eef to b65d2be May 20, 2019
@MatthewFlamm

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

I have added a fall back to parsing the metar code if the regular data is missing. This is to be consistent with #23875, as well as fixes issues with some stations having no formatted data but valid metar codes sometimes. I also changed it to only retrieve the latest observation since we aren't using prior observations.

The biggest mismatch in approach I see now with #23875 , is with obtaining the condition to convert to HA. This PR uses the icon url which has a published catalog of possible values and meanings. The sensor PR parses a text description.

@rewsssiestedbo

This comment has been minimized.

Copy link

commented May 20, 2019

I have added a fall back to parsing the metar code if the regular data is missing. This is to be consistent with #23875, as well as fixes issues with some stations having no formatted data but valid metar codes sometimes. I also changed it to only retrieve the latest observation since we aren't using prior observations.

The biggest mismatch in approach I see now with #23875 , is with obtaining the condition to convert to HA. This PR uses the icon url which has a published catalog of possible values and meanings. The sensor PR parses a text description.

It is probably best if I just remove the code to translate the NWS textDescription values to HA condition codes. None of the other weather sensors use the HA condition codes, and unlike for the weather platforms, there is nothing that depends upon the specific HA condition codes. I had mainly put it in as a basis for a future weather platform module, but since you have that module completed, there is no reason to keep it.

@rewsssiestedbo

This comment has been minimized.

Copy link

commented May 24, 2019

The code doesn't handle the exceptions that can be raised from the pynws methods: if the longitude/latitude is outside the area covered by the NWS, the API will return an HTTP 404 error, which pynws turns into a ClientReponseError exception. The same thing happens if the station code is not valid. These should be turned into a ConfigEntryNotReady exception. Other types of errors from the pynws methods typically indicate an issue with the connection to the API (or between the Akamai edge server and the NWS backend), and should turn into a PlatformNotReady (on any call during the initial setup), or treated as a missed poll during normal updates.

@rewsssiestedbo

This comment has been minimized.

Copy link

commented May 25, 2019

I starting running this component yesterday. Today it logged a number of errors:

  File "/home/homeassistant/.homeassistant/custom_components/nws/weather.py", line 268, in wind_bearing
    wind_bearing = self._metar_obs.wind_dir.value()
AttributeError: 'NoneType' object has no attribute 'value'

This appears to be a result of the wind direction value in the JSON being null, and the field in the METAR data saying VRB (variable), which results in no value from the parsed metar record either. An example of this is the observation data for KPHL at "https://api.weather.gov/stations/KPHL/observations/2019-05-23T20:02:00+00:00"

@MatthewFlamm

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

AttributeError: 'NoneType' object has no attribute 'value'

This should be fixed now. I'll work on handling errors especially during platform setup as these seem to be fatal.

@MatthewFlamm MatthewFlamm force-pushed the MatthewFlamm:add_nws branch from ca54fa1 to 75c5488 May 31, 2019
@MatthewFlamm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

@fabaff I feel that the code here has ballooned too much. I'm working on pulling a lot of the messy logic into the upstream pypi library to provide a cleaner interface. This is almost done. Once that is complete, should I push here, or open a new PR?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

I would say, continue here, thanks!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

If it's going to take a while until the new changes are ready, please close the PR, then reopen it when you're ready for a review. We're trying to decrease our open PR buffer. Thanks!

@MatthewFlamm

This comment has been minimized.

Copy link
Contributor Author

commented Jul 14, 2019

Thanks, Changes are very close to being finished, I'll continue here

MatthewFlamm added 8 commits May 3, 2019
Use metar code from nws observation if normal api is missing data.
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 10, 2019

Good! Now it's just the tests left to either adjust per my comment above, or remove them and exclude the integration in .coveragerc.

@MatthewFlamm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

Thanks! I found the ipma integration had tests in pytest form that I can try working off of. I will give it a shot, but if I'm making no progress, I will take the second route.

@sofar

This comment has been minimized.

Copy link

commented Aug 23, 2019

I'm still really looking forward to using this from stock. I've been using it as a local integration for a while without issues and while I'm probably not running the latest, it's been well worth it - this beats many of the other services that require a subscription.

@MatthewFlamm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

Is there anything else needed here?

@sofar If this doesn't move, I'll update the custom component repo to support HACS so it is easier to stay on top of versions.

@MatthewFlamm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

At risk of setting this PR back even farther, I have a working config flow setup for this integration that basically just as asks for all the fields in the configuration. I'm trying to figure out how to get some of the options into an option flow witha list of possible values.

For example,.I set lat/long in the config flow. Then I set up my entity with the default weather station for the lat/lon. The options flow ideally would get a list of nearly stations either using API call, or even better taking it from the entity directly, and provide a list to the user. I cannot find any examples of this usage flow however. I only see text forms and check boxes. It doesn't even seem possible to dynamically update the entry names on the data flow form, which would be a hack method anyway.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 23, 2019

Let's not add anything more to this PR. That can come in the next PR.

Copy link
Member

left a comment

Great!

Dev automation moved this from Review in progress to Reviewer approved Aug 23, 2019
@MartinHjelmare MartinHjelmare merged commit 17750a6 into home-assistant:dev Aug 23, 2019
9 checks passed
9 checks passed
CI Build #20190816.1 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
Dev automation moved this from Reviewer approved to Done Aug 23, 2019
@MatthewFlamm

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

Thank you for your reviewing time and support @MartinHjelmare, @fabaff, and @rewsssiestedbo.

@MatthewFlamm MatthewFlamm deleted the MatthewFlamm:add_nws branch Aug 23, 2019
@balloob balloob removed the new-platform label Sep 11, 2019
@balloob balloob referenced this pull request Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Done
7 participants
You can’t perform that action at this time.