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

Google Places Search Lookup #1143

Merged
merged 12 commits into from
Jan 31, 2017
Merged

Conversation

waruboy
Copy link
Contributor

@waruboy waruboy commented Jan 13, 2017

Hello. This is my attempt to integrate gogole places search lookup, as discussed on #1128 .

This query return way less data than other google query though, so many method will just return blank.
What do you think?

@waruboy
Copy link
Contributor Author

waruboy commented Jan 23, 2017

Hmm, I have no idea why the rbx based CI test is failing. Something about the gemset which I don't touch

@alexreisner
Copy link
Owner

Ah, sorry, don't worry about that failing test. Nothing to do with what you did, which looks great! I just need to find the time to go over it more carefully--at the moment the only thing I can spot that's missing is an entry in the README.

@waruboy
Copy link
Contributor Author

waruboy commented Jan 23, 2017

@alexreisner Thanks for responding! No pressure from here, I know it can be though maintaining an OSS project. Ah yes, will draft something to add in the README

@alexreisner
Copy link
Owner

Looks good. One more thing: is it necessary to define the results method in the lookup? It looks the same as the method in the lookup it's inheriting from, but maybe I'm missing something subtle...

@waruboy
Copy link
Contributor Author

waruboy commented Jan 29, 2017

@alexreisner Ah yeah, only difference is just the log message when raising error.
I followed the style of GooglePlaceDetails lookup

@alexreisner
Copy link
Owner

Ah, right. In that case I think it'd be better to make the superclass's method insert the name of the lookup dynamically into the error message. There's too much critical functionality in there to copy and paste. It will have the side effect of slightly improving the Google Premier lookup's error message, too.

@waruboy
Copy link
Contributor Author

waruboy commented Jan 29, 2017

@alexreisner good idea. Should we make it another PR or just merge it here? Would be happy to do either way

@alexreisner
Copy link
Owner

Here is good. Thanks!

@waruboy
Copy link
Contributor Author

waruboy commented Jan 30, 2017

@alexreisner done!

@alexreisner
Copy link
Owner

Ah, sorry, I should have been more specific. Just call the name method on the lookup for the human-readable name that should go in the error message. That make sense?

@waruboy
Copy link
Contributor Author

waruboy commented Jan 30, 2017

@alexreisner woops, totally forgot that we have that method. Is this what you mean?

@aceunreal
Copy link

Is it worth also adding a google_premier_places_search lookup?

@waruboy
Copy link
Contributor Author

waruboy commented Jan 31, 2017

@aceunreal I don't know. Does it need different request? The premium data feature will be deprecated though
https://developers.google.com/places/web-service/search#premium-data

@aceunreal
Copy link

@waruboy when using a Google premium plan, you have the possibility of using a signature as the authentication. eg: https://github.com/alexreisner/geocoder/blob/master/lib/geocoder/lookups/google_premier.rb

Premium data is different from premium plan and I do not believe these will be deprecated anytime soon.

@waruboy
Copy link
Contributor Author

waruboy commented Jan 31, 2017

@aceunreal Ah, I think it is not applicable for google places.
From: https://developers.google.com/places/web-service/search

Note for Google Maps APIs Premium Plan customers: You must include an API key in your requests. You should not include a client or signature parameter with your requests.

Or is this different with what you are talking?

@aceunreal
Copy link

@waruboy 👍 Looks like client/signature is not supported for Google places

@alexreisner alexreisner merged commit a3518a4 into alexreisner:master Jan 31, 2017
@waruboy
Copy link
Contributor Author

waruboy commented Jan 31, 2017

Thanks 😄

aceunreal pushed a commit to aceunreal/geocoder that referenced this pull request May 17, 2017
ChangsoonKim pushed a commit to ChangsoonKim/geocoder that referenced this pull request Nov 23, 2017
zacviandier pushed a commit to teezily/geocoder that referenced this pull request Feb 9, 2018
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.

3 participants