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

[OpenWeatherMap] Convert OpenWeatherMap to use the OneCallAPI #8253

Closed
wants to merge 4 commits into from

Conversation

MPH80
Copy link

@MPH80 MPH80 commented Aug 4, 2020

Converts over the OpenWeatherMap to use the OneCallAPI function rather than the existing separate calls for daily/weekly forecasts.

This brings in several benefits - most notably the daily forecasts, which you can't get under the old calls without paying for them.

The downsides:
It removes any reference to the station location and ID.
It also removes access to the Wind Gust

There's still work to do - comments to add etc - but I want to expose this here to allow debate on how to proceed with the UVIndexHandler since removing this would break existing channels. We could add the UV Index into the weather (which is where it can be found now) - it's just a matter of adding the channels to the relevant XML config (and leave the existing objects and calls) - or we can somehow pass the data between the handlers to prevent the second call.

OR we can just get rid of the UVIndexHandler and break the channels!

Signed-off-by: Michael Hazelden <michael@hazelden.me>
@TravisBuddy
Copy link

Travis tests were successful

Hey @MPH80,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

various forecasts and current. Restoring Wind Gust to current. Also
adding comments to the PoJos

Signed-off-by: Michael Hazelden <michael@hazelden.me>
Signed-off-by: Michael Hazelden <michael@hazelden.me>
@MPH80
Copy link
Author

MPH80 commented Aug 5, 2020

As an update - I performed a more detailed review over the responses, added back wind gust to the current forecast and included visibility into all 3 objects. I also decided to put the UV Index into each of the forecasts. I felt exposing the data there was better than not and felt individuals could decide which to use. Finally, on additions, I've added the probability of precipitation where it was available too (this seems to be a new field). All my testing is seeming that this is working - so this should be heading to a point where it's no longer a WIP.

@TravisBuddy
Copy link

Travis tests have failed

Hey @MPH80,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

@TravisBuddy
Copy link

Travis tests were successful

Hey @MPH80,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

1 similar comment
@TravisBuddy
Copy link

Travis tests were successful

Hey @MPH80,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

Modifying config to prevent more than 48 hours being requested.
Ensuring Gust Speed and UV Index are on the forecasted objects
Bringing readme up to date.

Signed-off-by: Michael Hazelden <michael@hazelden.me>
@TravisBuddy
Copy link

Travis tests were successful

Hey @MPH80,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@MPH80 MPH80 changed the title [OpenWeatherMap][WIP] Convert OpenWeatherMap to use the OneCallAPI [OpenWeatherMap] Convert OpenWeatherMap to use the OneCallAPI Aug 7, 2020
@Wolfgang1966
Copy link

Sorry for the long delay in commenting. One general idea: How about not modifying the existing things but instead adding a new one that uses the one call api. That way users of the existing things are not bothered by losing functionality and are able to make use of the advantage of their paid accounts.

I will try to spend some time on this end of August in my vacation.

@MPH80
Copy link
Author

MPH80 commented Sep 4, 2020

Being superceded by #8395

@MPH80 MPH80 closed this Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on (potentially) not backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants