-
-
Notifications
You must be signed in to change notification settings - Fork 20
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
Exposed the national weather alerts response in the One Call API #25
Conversation
Added national alerts section to VCR fixture. Added check for each alert field value.
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.
Thanks! Made a bunch of comments, this is close.
CHANGELOG.md
Outdated
@@ -2,6 +2,7 @@ | |||
|
|||
* [#21](https://github.com/dblock/open-weather-ruby-client/pull/21), [#20](https://github.com/dblock/open-weather-ruby-client/pull/20), [#19](https://github.com/dblock/open-weather-ruby-client/pull/19), [#18](https://github.com/dblock/open-weather-ruby-client/pull/18): Added support for Stations API - [@wasabigeek](https://github.com/wasabigeek). | |||
* [#22](https://github.com/dblock/open-weather-ruby-client/pull/23): Removed API version from `Config#endpoint` - [@dblock](https://github.com/dblock). | |||
* Exposed the national weather alerts response in the One Call API [@troya2](https://github.com/troya2) |
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.
Fix the format and danger will stop complaining. Missing a -
, a period and a PR link in front.
property 'end', transform_with: ->(v) { Time.at(v).utc } # UTC | ||
property 'description' | ||
|
||
def initialize(args = nil, options = {}) |
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.
You don't need to override the constructor unless you're doing something extra here. Just remove it.
@@ -95,7 +95,10 @@ http_interactions: | |||
rain","icon":"10d"}],"clouds":99,"rain":8.01,"uvi":10.95},{"dt":1589824800,"sunrise":1589800419,"sunset":1589850710,"temp":{"day":298.65,"min":289.07,"max":300.81,"night":289.49,"eve":297.92,"morn":289.07},"feels_like":{"day":298.14,"night":288.35,"eve":298.74,"morn":288.15},"pressure":1015,"humidity":56,"dew_point":289.47,"wind_speed":3.6,"wind_deg":21,"weather":[{"id":802,"main":"Clouds","description":"scattered | |||
clouds","icon":"03d"}],"clouds":49,"uvi":10.91},{"dt":1589911200,"sunrise":1589886783,"sunset":1589937154,"temp":{"day":296.27,"min":286.83,"max":298.28,"night":288.9,"eve":297.35,"morn":286.83},"feels_like":{"day":295.57,"night":288.28,"eve":299.19,"morn":286.03},"pressure":1015,"humidity":56,"dew_point":287.09,"wind_speed":2.73,"wind_deg":40,"weather":[{"id":800,"main":"Clear","description":"clear | |||
sky","icon":"01d"}],"clouds":0,"uvi":10.01},{"dt":1589997600,"sunrise":1589973148,"sunset":1590023597,"temp":{"day":298.21,"min":287.71,"max":299.71,"night":290.03,"eve":298.59,"morn":287.71},"feels_like":{"day":299.68,"night":289.89,"eve":301.62,"morn":287.74},"pressure":1014,"humidity":61,"dew_point":290.18,"wind_speed":1.3,"wind_deg":71,"weather":[{"id":800,"main":"Clear","description":"clear | |||
sky","icon":"01d"}],"clouds":0,"uvi":10.16}]}' | |||
sky","icon":"01d"}],"clouds":0,"uvi":10.16}],"alerts": [{"sender_name": "NWS Tulsa (Eastern Oklahoma)","event": "Heat Advisory","start": 1597341600,"end": 1597366800,"description": "...HEAT ADVISORY REMAINS IN EFFECT FROM 1 PM THIS AFTERNOON TO\n8 PM CDT THIS EVENING...\n* WHAT...Heat index values of 105 to 109 degrees expected.\n* WHERE...Creek, Okfuskee, Okmulgee, McIntosh, Pittsburg,\nLatimer, Pushmataha, and Choctaw Counties.\n* WHEN...From 1 PM to 8 PM CDT Thursday.\n* IMPACTS...The combination of hot temperatures and high\nhumidity will combine to create a dangerous situation in which\nheat illnesses are possible."}]}' |
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 usually don't hand-craft VCR-recorded data. You should record a new separate VCR K7 .yml that has alerts data and write a separate unit test for that.
Any interest in finishing this @troya2? |
@dblock Thanks for the prod - looking at it now. I haven't used VCR, so I'll have to work on that. The other items are easy. Committing them now. |
@dblock I believe I've resolved all of your comments. Let me know if not. Happy new year! |
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.
CHANGELOG is in the wrong place, the rest looks good!
CHANGELOG.md
Outdated
@@ -12,6 +12,8 @@ | |||
* [#27](https://github.com/dblock/open-weather-ruby-client/pull/27): Removed default values for Faraday’s SSL settings ca_file and ca_path - [@sunny](https://github.com/sunny). | |||
* [#21](https://github.com/dblock/open-weather-ruby-client/pull/21), [#20](https://github.com/dblock/open-weather-ruby-client/pull/20), [#19](https://github.com/dblock/open-weather-ruby-client/pull/19), [#18](https://github.com/dblock/open-weather-ruby-client/pull/18): Added support for Stations API - [@wasabigeek](https://github.com/wasabigeek). | |||
* [#22](https://github.com/dblock/open-weather-ruby-client/pull/23): Removed API version from `Config#endpoint` - [@dblock](https://github.com/dblock). | |||
* [#25](https://github.com/dblock/open-weather-ruby-client/pull/25): Exposed the national weather alerts response in the One Call API [@troya2](https://github.com/troya2). |
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.
This should go to the 0.4.1 next release above, please. Rebase.
|
||
# Alerts | ||
expect(data.alerts.first).to be_a OpenWeather::Models::OneCall::Alert | ||
expect(data.alerts.first.sender_name).to eq 'NWS Phoenix (Central Arizona and California Desert)' |
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.
Nitpicking, but we could do this, which may be slightly more elegant.
data.alerts.first.tap do |alert|
expect(alert).to be_a
expect(alert.sender_name).to eq ...
end
Added national alerts section to VCR fixture. Added check for each alert field value.
…nd updated expectations to match.
Thanks for finishing this up! Want to help with co-maintenance of this gem? Make the next release? Drop me a note to dblock at dblock dot org with your rubygems username if yes. |
Added the feature, updated the relevant fixture, and added supporting specs.