Skip to content
This repository has been archived by the owner on Jun 20, 2022. It is now read-only.

Implement '/meta' #118

Merged
merged 6 commits into from
Aug 12, 2015
Merged

Implement '/meta' #118

merged 6 commits into from
Aug 12, 2015

Conversation

pcasaretto
Copy link
Contributor

Hey guys,

Learning go and currently using go-octokit as a dependency for another project.
I wanted to contribute back and also scratch my own itch :) .
Opening this PR with WIP to get early feedback.
The code currently works.

Some specific questions on the code itself.

Thanks!

// Get the user search results based on MetaService#URL
//
// https://developer.github.com/v3/meta/#meta
func (m *MetaService) One() (meta Meta, result *Result) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is One a good name for this function?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for checking. One isn't a great fit here, is it? Wondering if we should forgo the service and just have client.Meta return an ApiInfo struct? Thoughts?

@brodock
Copy link

brodock commented Aug 9, 2015

@pengwynn can you please provide some feedback here? We are implementing other missing pieces and would like to know if you are interested in more PRs.

@pengwynn
Copy link
Collaborator

@pcasaretto @brodock: My apologies, I was on vacation when this landed. Was buried in my ⭐ folder.

I left some feedback. Thanks for the patch!

Included a draft for the tests
@pcasaretto
Copy link
Contributor Author

Thanks for the feedback @pengwynn. Tried to incorporate all of it in the most recent commit.
Apart from the obvious error cases, should I focus on any specific test cases?

)

// Meta return an APIInfo with the current API meta information
func (c *Client) Meta(uri *Hyperlink) (info APIInfo, result *Result) {
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 was a little confused if I should accept a *url.URL, a *Hyperlink, or nothing at all here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this looks good.

@pcasaretto pcasaretto changed the title [WIP] First pass at implementing '/meta' [WIP] Implement '/meta' Aug 11, 2015
@pcasaretto
Copy link
Contributor Author

I've added tests for the basic cases, but there is lack of coverage where the IPs and IPNets get parsed.
Do you think it's important to test that?
I did try but I could't stub the request on the same test and had to add two different json fixtures. I figured it wasn't worth the trouble.

@pcasaretto pcasaretto changed the title [WIP] Implement '/meta' Implement '/meta' Aug 12, 2015
@pengwynn
Copy link
Collaborator

I've added tests for the basic cases, but there is lack of coverage where the IPs and IPNets get parsed.
Do you think it's important to test that?

I think this is fine for now. Thanks for the patch. 🍰

pengwynn added a commit that referenced this pull request Aug 12, 2015
@pengwynn pengwynn merged commit a605346 into octokit:master Aug 12, 2015
@pcasaretto pcasaretto deleted the meta-api branch August 12, 2015 20:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants