-
-
Notifications
You must be signed in to change notification settings - Fork 32.7k
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
Forecast.io: Added minutely, hourly, and daily summary #1943
Forecast.io: Added minutely, hourly, and daily summary #1943
Conversation
pylint issue: Otherwise work and looks ok for me. |
I'm not sure how to fix that, I'm still pretty new to python. My guess is I need to put everything in self.data instead of adding "self.data_minutely", "self.data_hourly", and "self.data_daily". |
You could add Perhaps it's worth to think to only work with Can you remove the unused import? Seems to be a left-over from the past. |
I think that import needs to be there, otherwise I get the following:
|
Sorry, my bad...the import is needed for catching the exception. |
@@ -134,11 +137,20 @@ def update(self): | |||
import forecastio | |||
|
|||
self.forecast_client.update() | |||
data = self.forecast_client.data | |||
data = self.forecast_client.data.currently() | |||
data_minutely = self.forecast_client.data.minutely() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't fetch this data unless we need it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or does it work just like currently
and does it not cause an extra request to the server ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the python-forecastio docs when we call forecastio.load_forecast() we get all the data, so it shouldn't be any extra requests.
Looks good! 🐬 |
Description:
Additional summary types, shown on Forecast.io as "next hour", "next 24 hours" and "next 7 days".
Related issue (if applicable): #
Example entry for
configuration.yaml
(if applicable):Checklist:
If code communicates with devices:
tox
run successfully. Your PR cannot be merged unless tests passREQUIREMENTS
variable (example).requirements_all.txt
by runningscript/gen_requirements_all.py
..coveragerc
.If the code does not interact with devices:
tox
run successfully. Your PR cannot be merged unless tests pass