-
Notifications
You must be signed in to change notification settings - Fork 482
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
Update weather script #32384
Update weather script #32384
Conversation
WeatherRecord = Struct.new(:id, :city, :state, :timestamp, :temp_min, :temp_max, :weather, :icon, :wind_speed) | ||
|
||
def cardinal_dir_from_degrees(degrees) | ||
dirs = %w(N NE E SE S SW W NW N) |
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.
Is it intentional that N
is in here twice?
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.
Oh, I see. For 0 and 360.
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.
Ok, second question. Is it ever possible to get degrees above 360 or below 0?
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.
Yes, intentional for N
twice, for 0 and 360 as you said :)
I don't think the API response will ever have numbers outside that range.
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.
Ok. What about error input? If degrees is "nil" or something similar, will it cause the script to error out and exit early? Is that ok?
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.
✔️ Nice update
bin/cron/applab_datasets/weather
Outdated
@@ -165,6 +197,7 @@ end | |||
def main | |||
fb = FirebaseHelper.new('shared') | |||
records, columns = get_weather_data | |||
fb.delete_shared_table('weather') | |||
fb.upload_shared_table('weather', records, columns) |
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.
Is there a possibility that students querying the weather table will get nothing because it's been deleted but not re-uploaded? Is this a problem?
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.
If someone happens to be looking at the table when the script is run (once per day), then yes I think they will see nothing for a fraction of a second. But then the new data will pop in automatically when the new table is uploaded. To me that doesn't seem like a big deal, especially for v1, but what do you think?
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.
✔️ ^ I agree. Since it auto-populates, it's not a big deal.
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.
✔️ ^ I agree. Since it auto-populates, it's not a big deal.
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.
LGTM!
"cats", | ||
"dogs", | ||
"states", | ||
"weather", |
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.
Did you intend to keep weather
?
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.
fixed!
Description
Switch to using the daily API per curriculum request: https://openweathermap.org/forecast16
![image](https://user-images.githubusercontent.com/8787187/70653660-c397a480-1c09-11ea-9f9c-50fb84e61f9e.png)
Before:
After:
![image](https://user-images.githubusercontent.com/8787187/72277681-33bd8f80-35e7-11ea-945b-6874bf4623c0.png)
Testing story
Manual testing only
Reviewer Checklist: