Add register stations #18

wasabigeek · 2020-05-30T08:44:14Z

Start support for stations (#10) by adding a register_station method to Client (https://openweathermap.org/stations#create_station).

Opening for early feedback, some questions:

should all the endpoints go into the client, or should the more RESTful resources be a separate component?
stations seem to require a minimum API version of 3, I could add some checks on the version URL when attempting to use the stations endpoints, what do you think?
registering stations requires JSON requests, I hacked something around for now as the library only allows form requests, wondering if you had advice on the best way to approach this?

dblock · 2020-05-30T11:28:30Z

Start support for stations by adding a register_stations method to Client (https://openweathermap.org/stations#create_station).

Cool. Thanks!

should all the endpoints go into the client, or should the more RESTful resources be a separate component?

To be consistent I think the client should have register_station, get_station, update_station, etc., but I would also add methods to the Station model, e.g. register!, update! or destroy! that call those, e.g. client.register_station(self.to_h) or something like that.

stations seem to require a minimum API version of 3, I could add some checks on the version URL when attempting to use the stations endpoints, what do you think?

Maybe, I wouldn't care enough. I would just call out that this library also now supports some 3.0 beta functions. If you use 2.5 endpoints for station it's going to give you an error anyway (write a test).

registering stations requires JSON requests, I hacked something around for now as the library only allows form requests, wondering if you had advice on the best way to approach this?

This doesn't seem right. The OpenWeatherMap docs say that if you pass Content-Type: application/json in the HTTP request you'll get back JSON. Either way adding endpoint-specific parameters belongs in the endpoint method, not in the middleware.

This seems unnecessary. I think 2.5 API never uses POST, so you can just default it to application/json. I could be wrong, in which case I would pass a content_type option instead of use_json, but that's a small implementation detail.

dblock

The OO interface is not quite an OO interface, but almost there!

README.md

dblock · 2020-05-31T23:15:50Z

lib/open_weather/models/model.rb

@@ -10,7 +10,8 @@ class Model < Hashie::Trash
      attr_reader :options

      def initialize(args = nil, options = {})
-        super args
+        transformed_args = args.respond_to?(:transform_keys) ? args.transform_keys(&:to_s) : args


Care to explain?

A symbol-keyed hash was “ignored” by the model, I think because a Hashie::Dash property will account for whether it’s defined as a string or symbol (though there didn’t seem to be an issue retrieving the property via a method)

it's possible that the model class needs a merge initializer instead. Either way make sure to have a spec for this and we can fix it later.

Finding this hard to test on Model itself, would it be sufficient to test Station for now?

dblock · 2020-05-31T23:16:52Z

lib/open_weather/models/station.rb

+      property 'updated_at' # timestamp when station was updated
+      property 'rank' # rank of station
+
+      def self.register!(attributes)


This becomes a method that can take additional options:

def register!(options = {}) OpenWeather::Client.new.register_station(attributes, options) end

dblock · 2020-05-31T23:18:18Z

spec/lib/open_weather/models/station_spec.rb

+
+require 'spec_helper'
+
+RSpec.describe OpenWeather::Models::Station do


Probably shorter to use the same VCR here. The problem with the double is that it will continue working even if we break the API call.

Curious, since that client method is unit-tested already, wouldn't it be sufficient to unit-test this as well (check that we call client with the right params)?

It's sufficient, but implies behavior and IMHO hides bugs. I've debugged my fair share of double's to avoid them, more often than to use them. That said no strong opinions.

dblock · 2020-05-31T23:19:10Z

spec/open_weather/endpoints/stations_spec.rb

+  include_context 'API client'
+
+  it 'registers a station', vcr: { cassette_name: 'stations/register_success' } do
+    client.endpoint = 'https://api.openweathermap.org/data/3.0'


Let's extend the context to take options like so:

include_context 'API client', endpoint: 'https://api.openweathermap.org/data/3.0'

Does this work out-of-the-box? It seems like we can only pass a block https://relishapp.com/rspec/rspec-core/docs/example-groups/shared-context#declare-a-shared-context,-include-it-with-%60include-context%60-and-extend-it-with-an-additional-block
Well, TIL

dblock · 2020-05-31T23:20:31Z

spec/open_weather/endpoints/stations_spec.rb

+      altitude: 150
+    )
+    expect(data).to be_a(OpenWeather::Models::Station)
+    expect(data).to have_attributes(


As long as have_attributes starts failing if I add an extra attribute, this works, but it is inconsistent with other tests. Not sure if I care about the latter much, just thinking out loud.

It does fail, but perhaps with a more cryptic error:

1) stations registers a station Failure/Error: expect(data).to have_attributes( ... wazzoo: 'test' ) expected {...} to respond to :wazzoo with 0 arguments

So that's better, it lets us check all attributes. I wouldn't mind if all the other tests changed ;)

So that's better, it lets us check all attributes. I wouldn't mind if all the other tests changed ;)

Can I open a separate PR for that 🤣

dblock

Let's see the model change and I'm happy with this.

dblock · 2020-06-01T12:13:36Z

lib/open_weather/models/station.rb

+module OpenWeather
+  module Models
+    class Station < Model
+      # internal identifier for the station


Be consistent, put the comment after the code line.

My bad, I was trying to see if it could work with both 'id' and 'ID' and thought the line was too long

You can probably alias it if you really want to.

dblock · 2020-06-01T12:16:09Z

spec/support/shared/api_client.rb

  before do
    OpenWeather::Config.reset
+    OpenWeather::Config.endpoint = params[:endpoint] if params&.key?(:endpoint)


For a future PR, I'd like to make it future proof able to write OpenWeather::Client.configure(params) here.

dblock · 2020-06-01T14:18:15Z

lib/open_weather/models/station.rb

+      property 'rank' # rank of station
+
+      def register!
+        data = OpenWeather::Client.new.register_station(to_h)


For future update, we can save the extra instance construction with OpenWeather::Client.new.post('stations', options). I think that's OK to duplicate.

dblock · 2020-06-01T14:19:43Z

Good stuff, merged.

Want to add the rest of station methods?

wasabigeek · 2020-06-01T14:36:09Z

Want to add the rest of station methods?

Will slowly chip at them!

wasabigeek added 3 commits May 30, 2020 15:04

Add register_stations placeholder and spec

defa749

Add Station model

ca684c5

Allow json request

71c0980

wasabigeek added 12 commits May 30, 2020 20:52

Make all requests default to JSON

675503d

Remove options from Stations#register_station

09239ee

Translate ID from API response

86ecc7c

Start on #create! method

e301134

Update Station model after a create!

ed74b9d

refactor: Move hash transformation to Model

339a5f1

Rename method to register!

354dcad

Make register! a class method instead

9ea28cc

Update README to include stations API

a8f93a8

Update changelog

6033cc9

doc: Update TOC in README

1211ec4

Fix typo in README

703771b

dblock requested changes May 31, 2020

View reviewed changes

wasabigeek added 4 commits June 1, 2020 20:11

refactor: Let shared_context take params

7f7acc7

refactor: Use VCR in spec

d1ae475

feat: Make register! an instance method

c1d0b87

refactor: Use params for include_context

728b116

dblock requested changes Jun 1, 2020

View reviewed changes

wasabigeek added 3 commits June 1, 2020 20:46

Update README.md

14623af

Make comment more consistent

ba32947

Fix readme

8e7e080

dblock reviewed Jun 1, 2020

View reviewed changes

dblock merged commit 79ee36f into dblock:master Jun 1, 2020

wasabigeek deleted the add-register-stations branch June 1, 2020 14:34

dblock mentioned this pull request Jun 3, 2020

Add support for weather stations #10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add register stations #18

Add register stations #18

wasabigeek commented May 30, 2020 •

edited

Loading

dblock commented May 30, 2020 •

edited

Loading

dblock left a comment

dblock May 31, 2020

wasabigeek Jun 1, 2020

dblock Jun 1, 2020

wasabigeek Jun 1, 2020

dblock Jun 1, 2020

dblock May 31, 2020

dblock May 31, 2020

wasabigeek Jun 1, 2020

dblock Jun 1, 2020

dblock May 31, 2020

wasabigeek Jun 1, 2020 •

edited

Loading

dblock May 31, 2020

wasabigeek Jun 1, 2020 •

edited

Loading

dblock Jun 1, 2020

wasabigeek Jun 1, 2020

dblock left a comment

dblock Jun 1, 2020

wasabigeek Jun 1, 2020

dblock Jun 1, 2020

dblock Jun 1, 2020

dblock Jun 1, 2020

dblock commented Jun 1, 2020

wasabigeek commented Jun 1, 2020


		require 'spec_helper'

		RSpec.describe OpenWeather::Models::Station do

Add register stations #18

Add register stations #18

Conversation

wasabigeek commented May 30, 2020 • edited Loading

dblock commented May 30, 2020 • edited Loading

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wasabigeek Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wasabigeek Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock commented Jun 1, 2020

wasabigeek commented Jun 1, 2020

wasabigeek commented May 30, 2020 •

edited

Loading

dblock commented May 30, 2020 •

edited

Loading

wasabigeek Jun 1, 2020 •

edited

Loading

wasabigeek Jun 1, 2020 •

edited

Loading