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

Add support for Google Maps API For Work params #45

Closed

Conversation

skateinmars
Copy link

The Maps API can be used with a Google business account but this require a different authentication method than a simple API key and request URL signing.

This Pull Request adds support for this auth method based on the documentation provided here: https://developers.google.com/maps/documentation/business/webservices/auth

I reused the same GoogleGeocoder but added 3 new vars and their matching configuration functions.


return queryStrBuffer.String(), nil
} else if GoogleClientID != "" && GooglePrivateKey != "" {
queryStrBuffer := bytes.NewBufferString(queryStr)
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if it makes sense to break these out into separate functions for building a URL depending on the authentication schema, be it API key or Business ClientId and PrivateKey.

Something like:

buildGoogleMapsClientSideQuery(params string) (string, error)
buildGoogleMapsForWorkQuery(params string) (string, error)
buildDefaultGoogleMapsQuery() (string)

@kellydunn
Copy link
Owner

Heya! Thanks for taking the time to make Pull Request 😄 , I appreciate it!!

This is definitely a change I can see benefitting a lot of people, so thanks for doing it! This is a relatively big change, and as such, I had a few thoughts before proceeding. I left a few comments inline, let me know what you think!

Making geocoder implementations has been the biggest source of PRs for this lib, and admittedly, it's the place that needs the most love. A few of these were made in haste without considering how other people might use them, or using different auth schemas. As such, I want to use this opportunity to clean up things a bit so we can have the cleanest implementation as possible!

Thanks again!

@skateinmars
Copy link
Author

Thanks for the feedback!

I agree I didn't really clean up the code when changing it, I'll try to refactor this properly using an Auth Schema param.

@skateinmars
Copy link
Author

Hello, I refactored the code to add your proposed AuthSchema field to the GoogleGeocoder struct.
This does clean up the code and will avoid any issue where both an API key and a Client ID are set, but this change is not backward compatible anymore as you are required to supply this auth param if you want to use the API Key authentication (whereas before you only had to set the GoogleAPIKey var).
This may be acceptable though depending on how you manage versioning for this lib?

@skateinmars
Copy link
Author

Hello, did you have time to review the changes? Anything else I can do to help?

@kellydunn
Copy link
Owner

@skateinmars heya! Sorry for the delay in responding, it's been a busy week or so for me.

I'm still making a decision on how to handle the backwards compatible changes. I started writing up a set of features I'd like to see in a v1.0.0 release of golang-geo here a few months back, and I think this would be a great candidate for that, especially since golang 1.5 recently launched with support for experimental vendoring.

I think your work might be a good starting point to create a v1 branch and start work on some backwards incompatible changes.

That being said, I might hold off on introducing your featureset into master at the moment in favor for making it the first step towards a better v1

How do you feel about that?

@skateinmars
Copy link
Author

Hello, I agree we shouldn't merge this to master since there are incompatible changes.
The changes are fine for a v1, for the moment we use a fork for our application so there is no rush for us to get this merged upstream.

@kellydunn
Copy link
Owner

Awesome! I've created a new branch, v1.0.0-work-in-progress to begin the work I mentioned.

I'll close this PR for now. @skateinmars, Would you mind opening up a new PR against the new v1.0.0-work-in-progress branch? I'll gladly merge it in :)

Thanks!

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.

None yet

2 participants