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

Control Google places search API price #1461

Merged
merged 2 commits into from
Aug 7, 2020

Conversation

maximilientyc
Copy link
Contributor

References issue: #1460

language: query.language || configuration.language
}
end

def fields(query)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default every fields are requested, to prevent any regression from developers already using the gem.

end

def default_fields
legacy = %w[id reference]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

id and reference fields were already available in the previous API response (Text search response), I added those 2 fields here to prevent any regression for developers already using the gem

@maximilientyc
Copy link
Contributor Author

@alexreisner this PR is ready for review 👍

@alexreisner
Copy link
Owner

This looks great! Thanks for preserving the existing behavior, even if it's not the most cost-effective. My only question is whether we need to add the root_attr argument to the results method. It doesn't seem to be used, and I'd rather not change the method signature if we can avoid it.

@maximilientyc
Copy link
Contributor Author

It's actually being used in the method body, here: https://github.com/alexreisner/geocoder/pull/1461/files#diff-ec56420a19546fb7982cb25cfcc5bc52R51.
I think there's something to be refactored with how the results from Google are handled for each component (place search, details and premium). For this PR I had the idea to touch only the Place search component, and in another PR work on the refactoring of the Google clients.
Let me know what you think.

@alexreisner
Copy link
Owner

Sorry, my comment wasn't clear. What I don't see is any code that calls results with a second argument. In other words, if the line you linked to above was changed to return doc['results'] would it break anything?

As for separate refactoring PR, sounds good. And if that PR would take advantage of this second results argument, then that's where the argument should be added.

@maximilientyc
Copy link
Contributor Author

Thanks for the clarification 👍

After quickly checking the components inheriting from Google class, each component is implementing a different root attr:

Component Root attr
GooglePremier results
GooglePlacesDetails result
GooglePlacesSearch candidates

The results method is defined twice, I think we should change that. Once in the Google class, once in GooglePlacesDetails < Google. A quick and efficient refactor to be done in that PR would be to have a second parameter to the result method root_attr as a mandatory parameter. Every component inheriting from Google will have to implement the following result method, for example:

def results(query)
  super(query, 'candidates')
end

@alexreisner
Copy link
Owner

That sounds like a good plan to me!

@maximilientyc
Copy link
Contributor Author

I just made the changes, it's ready for review again. Don't hesitate if anything is unclear.

@alexreisner
Copy link
Owner

Perfect! Thanks for the good work on this.

@alexreisner alexreisner merged commit 353a434 into alexreisner:master Aug 7, 2020
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