Skip to content

Add lookup for Google Places Details API (New) #1637

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

patchy-oss
Copy link

Overview

Adds a lookup for Google Places Details (New).

Changes

  • Added new base class for Google - GoogleBase.
  • Added a base class for new Google API - GoogleNew.
  • Added new lookup for Google places details (new).
  • Tests.

Notes

Most of the changes are copy-paste with small corrections from the existing google lookups. I've tried to unify both google APIs, but seems like there's not too much to unify... Also I think the GoogleBase class is kinda redundant, so I can duplicate configure_ssl! and supported_protocols methods in both Google and GoogleNew instead (or maybe you have a better solution).
Naming is tough 🙃

Google documentation:

@maokomioko
Copy link

@alexreisner Hi Alex, possible to merge you think?

@zjwhitehead
Copy link

@alexreisner do you mind taking a look at this?
It would be handy to have this support as Google pushes users to upgrade to this API.

@alexreisner
Copy link
Owner

Really sorry for the massive delay here. I fully intend to review and merge this. However, I think a big of restructuring is needed. (@patchy-oss if you're still interested, feel free to do this, or someone else can take it on.)

Basically, if the new Google API supersedes the old one (ie, the old one is deprecated), we should replace the current GooglePlacesDetails classes, rather than adding GooglePlacesDetailsNew. If it can't be backward-compatible with the old API, that's fine, and it will be released as part of a new minor version of Geocoder.

Ideally, we should avoid adding a GoogleBase class unless it's really usable by all GoogleX lookups. But even then, I'd prefer if that were added in a separate PR. Too many changes make things hard to review, which is a tiny bit of the reason for the delay on this.

Does that make sense? Thanks for your patience.

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.

4 participants