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

contrib/google.golang.org/api: add google api integration #313

Merged
merged 9 commits into from
Sep 11, 2018
Merged

Conversation

dd-caleb
Copy link
Contributor

Implements the API portion of: #312. A separate pull request will implement the Cloud portion (which uses this integration behind the scenes).

I made a Tree class which is a lot like a prefix tree where the "words" are path segments, and a map is used to store children instead of an array. Finally a regular expression is generated using a uritemplate package as a final check that a path matches.

The Tree is filled via a make_endpoints.go script which parses the JSON files in the package to discover every possible API endpoint.

@dd-caleb dd-caleb added apm:ecosystem contrib/* related feature requests or bugs feature-request labels Aug 10, 2018
@dd-caleb dd-caleb requested a review from gbbr August 10, 2018 17:48
Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

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

A bit "resourcey" but very nice and accurate! Consider unexporting the tree logic.


type (
// A Tree is a prefix tree for matching endpoints based on http requests.
Tree struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do any of these need to be part of the public API? I'm thinking not. In which case we can make them unexported. I'd check with godoc to make sure nothing extra is there and it's all easy to figure out from a user perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to make it private because the generator script is not in the same package. I could make it internal.

Copy link
Contributor

Choose a reason for hiding this comment

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

It has to be private because there is no use for this in the public API. If I'm wrong go ahead and keep it. Otherwise we should hide it. If we have to create an internal package for that then maybe we should.

Copy link
Contributor

@gbbr gbbr Sep 11, 2018

Choose a reason for hiding this comment

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

Let's keep it. It can't hurt, and if anyone ever wants to add some custom endpoints, they can...

@gbbr gbbr added this to the 1.3.0 milestone Sep 11, 2018
@gbbr gbbr merged commit b224bc6 into v1 Sep 11, 2018
@gbbr gbbr deleted the caleb/gcp branch September 11, 2018 11:02
@gbbr
Copy link
Contributor

gbbr commented Sep 11, 2018

Thanks Caleb! Added further commits and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants