Skip to content

Added channel parameter to request (issue #77)#97

Merged
markmcd merged 7 commits intogooglemaps:masterfrom
mwindsor-beoped:master
Aug 19, 2015
Merged

Added channel parameter to request (issue #77)#97
markmcd merged 7 commits intogooglemaps:masterfrom
mwindsor-beoped:master

Conversation

@mwindsor-beoped
Copy link
Copy Markdown
Contributor

Added support for a channel parameter to the GoogleDistanceMatrixRequest (rather than the context), to allow maximum flexibility to callers.

@googlebot
Copy link
Copy Markdown

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@mwindsor-beoped
Copy link
Copy Markdown
Contributor Author

I signed it!

On 3 August 2015 at 15:14, googlebot notifications@github.com wrote:

Thanks for your pull request. It looks like this may be your first
contribution to a Google open source project, in which case you'll need to
sign a Contributor License Agreement (CLA).

[image: 📝] Please visit https://cla.developers.google.com/
https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll

verify. Thanks.


Reply to this email directly or view it on GitHub
#97 (comment)
.

@googlebot
Copy link
Copy Markdown

CLAs look good, thanks!

@mwindsor-beoped
Copy link
Copy Markdown
Contributor Author

I've made the changes requested by domesticmouse

@markmcd
Copy link
Copy Markdown
Contributor

markmcd commented Aug 6, 2015

Before we merge this, can you explain the reason for implementing per-request rather than on the context level?

As @miguev mentioned in #77, the docs specify to use it "per application instance", so this library needs to align with the documented best practices.

@mwindsor-beoped
Copy link
Copy Markdown
Contributor Author

Of course.

Here is the bit from the docs:

"The channel value must be a static value assigned per application instance, and must not be generated dynamically. You may not use channel values to track individual users, for example."

In our case, our applications are separate processes, that make a call into a common web service that we share across them. This allows us to implement our own common behaviour across all of them. It's this common web service that uses the Google maps api.

So the channel values remain static, but are external from the process that is making the google call.

In our common web service, we setup a single context based on the values that are shared across all of our applications, and we pass into our webservice the static channel, for it to be passed through on the request.

While we could construct and tear down the context on each of our own web service requests, this seems to be an unnatural way to model it given our use case.

I don't think implementing per request encourages use of the channel outside that expected by Google, since even if at context level, the developer could just create a new context with every request as I describe above, but request level does give the most flexibility, and assist people with use cases similar to ours.

@markmcd
Copy link
Copy Markdown
Contributor

markmcd commented Aug 11, 2015

OK - I'm convinced :)

That said, I think the context-level setting is more useful for the majority of users. Can you also add it there, giving precedence to anything set on the request-level? (i.e. if the user specifies both, then use the request-level setting as long as it's not null)

Thanks for your patience.

@mwindsor-beoped
Copy link
Copy Markdown
Contributor Author

I've added the requested context-level setting for channel, with request-level override

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe this should be 'Only useful for Google Maps for Work clients.'

Thanks!

@mwindsor-beoped
Copy link
Copy Markdown
Contributor Author

Would have got this change made much quicker @domesticmouse if somewhere in the to and fro it had mentioned that you wanted a semi-colon switched for a full stop! Hard to spot!

Hopefully we are good to go @markmcd

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Typo here - should be "&channel=".

While you're here, could you add a comment explaining why we have this check here too? e.g. // Channel can be supplied per-request or per-context. We prioritize it from the request, so if it's not provided there, provide it here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also, please reformat the expression in the if statement :)

if (channelSet == false && channel != null && !channel.isEmpty()) {

@mwindsor-beoped
Copy link
Copy Markdown
Contributor Author

Changes made as requested by @markmcd and @broady

@markmcd
Copy link
Copy Markdown
Contributor

markmcd commented Aug 19, 2015

LGTM! Thanks again for your contribution and your patience.

LGTM

markmcd added a commit that referenced this pull request Aug 19, 2015
Added channel parameter to request (issue #77)
@markmcd markmcd merged commit 51d6aed into googlemaps:master Aug 19, 2015
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.

5 participants