Add ability to set fields in configuration for Google Places

[czlee]

Thanks so much for your work on this gem, @alexreisner!

This PR adds support for:

1. the fields parameter to :google_places_details
2. configuring fields in Geocoder.configure, for both :google_places_details and :google_places_search

(I know this is two things, but they're very tightly related hard-to-separate things!)

Limitation: If the Geocoder configuration has fields configured, you can't override it in the query with nil. You can override it with [], but this would add &fields=& to the query string (which does the same thing, but would look different to the cache). Open to thoughts on whether this should be modified.

---

Note on relationship with #1514 (@ericds): In principle they have a similar objective, and this PR includes half of #1514. However, #1514 also specifies a collection of default fields in google_places_details.rb. Instead, in this PR I propose falling back to nil, which will send a request to Google Places without the fields= key, which (at least when I do it) seems to just default to all of the keys available. Reasons I felt this was preferable:

• retains existing/backwards functionality (i.e. if fields isn't specified, this will do the same thing as currently)
• when the available Google Places API fields change, this gem doesn't have to be updated to match it

That said, I'd be down to discuss this with @ericds to arrive at an agreement on how this should work, for @alexreisner to consider.

[alexreisner]

Thanks for this. And apologies to @ericds for not responding to #1514 (I simply missed it). I do prefer this no-defaults approach for exactly the reasons you name. Would you mind adding a test or two, to make sure the fields get formatted properly in the query string? Also, what do you think about allowing the config to be overridden by using nil? I think that would be more expected/standard behavior (and would solve the cache problem you mentioned), but is there a reason not to do it?

[czlee]

Would you mind adding a test or two, to make sure the fields get formatted properly in the query string?

For sure! Will do (in the next week or so).

Also, what do you think about allowing the config to be overridden by using nil?

Yeah, I kind of agree that it's more intuitive behaviour. I think I shied away from it because it would have (at face) changed the existing behaviour of :google_places_search, which currently specifies default fields. Currently, it's:

def fields(query)
  query_fields = query.options[:fields]
  return format_fields(query_fields) if query_fields

  default_fields
end

So if you pass in

Geocoder.search("ChIJ32KaYX-AhYARNa93OffiHEk", lookup: :google_places_search, fields: nil)

it will fall back to the default_fields. If we changed it to

def fields(query)
  if query.options.has_key?(:fields)
    return format_fields(query.options[:fields])
  end

  if configuration.has_key?(:fields)
    return format_fields(configuration[:fields])
  end

  default_fields
end

Then the same fields: nil would send the request without the fields parameter (because fields(query) now returns nil), falling back to Google's defaults, which would be a change in behaviour.

The only way to fall back to default_fields would be to omit fields: completely in both query and configuration.

It's a very subtle change in behaviour, but I know that I sometimes use nil when automatically constructing keyword arguments in response to user input where I know it has effect as omitting the argument, so I worried that it might break (a minority of) existing callers.

This wouldn't matter for :google_places_details, because both the current and last-fallback behaviour is to fall back to Google's defaults (via nil), so this difference only comes into play if the new configuration[:fields] isn't set. But doing this for :google_places_details but not :google_places_search would mean the two handle fields differently, which could be counterintuitive in itself.

So... yeah, basically just a small backwards compatibility point for :google_places_search. I'd be happy with the change, but I can imagine some existing users might at least want to be notified. 🙂

[alexreisner]

Yep, good point! Let's make this a pull request for the v1.8 branch (1.8.0 will probably be the next release) and I'll mention it in the release notes.

[czlee]

Dumb question, but how do I run the tests? I'm used to rspec and don't really have experience with the standard library unit test framework, or anything that needs to be done to provide a test environment for gems 😅 Sorry if I missed documentation for this, I promise I looked!

Currently I get, from the directory /usr/local/bundle/bundler/gems/geocoder-<commit-hash>/test,

# ruby unit/lookups/google_places_details_test.rb
Traceback (most recent call last):
	2: from unit/lookups/google_places_details_test.rb:2:in `<main>'
	1: from /usr/local/lib/ruby/2.6.0/rubygems/core_ext/kernel_require.rb:54:in `require'
/usr/local/lib/ruby/2.6.0/rubygems/core_ext/kernel_require.rb:54:in `require': cannot load such file -- test_helper (LoadError)

but I'm just running it in my app dev environment, which presumably isn't what the Geocoder test environment needs.

(That's why I converted the PR to a draft: I haven't actually run the tests yet to see if they pass.)

[czlee]

This repeats code in GooglePlacesSearch; let me know if you think these methods should be abstracted out to a base class GooglePlaces < Google. I felt it was arguable whether this was enough to justify it.

[alexreisner]

Thanks for pointing this out. I agree that it's not enough to merit the complexity of a new base class.

[czlee]

The tests were a good idea, turns out the above-discussed modification actually caused it to fail to omit the &fields= parameter, and the new tests caught it 😅

[alexreisner]

Excellent! Tests are great for that.

This looks good to me. Will merge shortly.

Thanks for the good work on this.

[alexreisner]

Closing this manually (was merged into the v1.8 branch, which Github isn't auto-marking as "merged").