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 remaining stations endpoints #20

Merged
merged 10 commits into from
Jun 3, 2020

Conversation

wasabigeek
Copy link
Collaborator

Pending a "delete!" method on Station.

Next step, add measurements.


def delete_station(id)
delete("stations/#{id}")
nil
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dblock what would you suggest to return here? Currently it the delete method returns as empty string (response.body).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think nil is fine.

end

def update_station(id, options = {})
OpenWeather::Models::Station.new(put("stations/#{id}", options))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why, but it seems like the API requires external_id to be specified 🤷

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I could potentially raise an error if id was not defined, do you have a suggestion on how to define / where to keep errors?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a parameter is missing, raise ArgumentError, generally I don't specialize errors unless they can happen on all APIs (e.g. incorrect HTTP proxy)

Copy link
Owner

@dblock dblock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff. Comments:

CHANGELOG.md Outdated
@@ -1,6 +1,7 @@
### 0.2.1 (Next)

* Your contribution here.
* [#20](https://github.com/dblock/open-weather-ruby-client/pull/20): Add remaining stations endpoints - [@wasabigeek](https://github.com/wasabigeek).
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combine these into [#20](), [#19](), ...: Add support for weather stations - ...

README.md Outdated
@@ -222,6 +225,33 @@ To list all stations, you can call the client method:
data = client.list_stations # => Array[OpenWeather::Models::Station]
```

#### Get Station

To get a stations, you can call the client method:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove "you can", it goes without saying :) same below

README.md Outdated

To get a stations, you can call the client method:
```ruby
data = client.get_station('5ed2118acca8ce0001f1aeg1') # => OpenWeather::Models::Station
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: because you're not using data, just remove it, so client.get ..., more below like this

README.md Outdated
```
Alternatively, call `update!` on an instance of `Station`:
```ruby
model = OpenWeather::Models::Station.new(external_id: 'SF_TEST001', ...).register!
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put model.register! on its own line.

end

def update_station(id, options = {})
OpenWeather::Models::Station.new(put("stations/#{id}", options))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a parameter is missing, raise ArgumentError, generally I don't specialize errors unless they can happen on all APIs (e.g. incorrect HTTP proxy)


def delete_station(id)
delete("stations/#{id}")
nil
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think nil is fine.

@@ -42,4 +42,38 @@
)
end
end

describe '#get_station' do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I’ve been following this convention http://www.betterspecs.org/#describe (describe for the method under test, context for a scenario)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been schooled ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They’re aliases so all good either way!

@wasabigeek wasabigeek marked this pull request as ready for review June 3, 2020 13:19
@dblock dblock merged commit 335f812 into dblock:master Jun 3, 2020
@dblock
Copy link
Owner

dblock commented Jun 3, 2020

Merged, nice work!

Want to help out with gem maintenance? Make the next release? Drop me an email with your rubygems username/email to dblock at dblock dot org.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants