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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

#720 Initial commit for listing Plans and Plan Accounts #732

Merged
merged 14 commits into from Nov 14, 2017

Conversation

Projects
None yet
5 participants
@lackerman
Contributor

lackerman commented Sep 28, 2017

  • Includes basic testing for Listing Plans and the Accounts linked to a Plan (including stubs)

NOTE This is currently awaiting scrutiny as it's my first contribution to the project. I would appreciate any feedback (good or bad) and the 馃憤 if it's ok to carry on with the rest of the implementation for #720

#720 Initial commit for listing Plans and Plan Accounts
* Includes basic testing for Listing Plans and the Accounts linked to a Plan (including stubs)
@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Sep 28, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, 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 your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

googlebot commented Sep 28, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, 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 your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@googlebot googlebot added the cla: no label Sep 28, 2017

@dmitshur dmitshur requested a review from gmlewis Sep 28, 2017

@dmitshur

@lackerman, this is a fantastic start!

It seems that we'll need to make some new decisions for this issue, because these endpoints offer something no other GitHub API endpoints have offered in the past: the alternative stubbed endpoints. /cc @gmlewis

I will think a little more about this, but so far, I think we should factor out that stubbed bool argument from each method. Perhaps, instead, it should be a new option that we add to github.Client. What do you think about that?

Aside from that, I spotted and pointed out some minor issues with missing periods at ends of sentences, and missing ,omitempty JSON tag options.

Show outdated Hide outdated github/apps_marketplace.go Outdated
Show outdated Hide outdated github/apps_marketplace.go Outdated
Show outdated Hide outdated github/apps_marketplace.go Outdated
Show outdated Hide outdated github/apps_marketplace.go Outdated
@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Sep 28, 2017

Contributor

I signed it!

Contributor

lackerman commented Sep 28, 2017

I signed it!

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Sep 28, 2017

CLAs look good, thanks!

googlebot commented Sep 28, 2017

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Sep 28, 2017

@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Sep 28, 2017

Contributor

@shurcooL Regarding

...I think we should factor out that stubbed bool argument from each method. Perhaps, instead, it should be a new option that we add to github.Client. What do you think about that?...

I'm so happy you said that. The flag felt like suck a hack. Not sure I have enough context or experience to comment on the preferred solution, but I'm happy to implement it ;).

Contributor

lackerman commented Sep 28, 2017

@shurcooL Regarding

...I think we should factor out that stubbed bool argument from each method. Perhaps, instead, it should be a new option that we add to github.Client. What do you think about that?...

I'm so happy you said that. The flag felt like suck a hack. Not sure I have enough context or experience to comment on the preferred solution, but I'm happy to implement it ;).

@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Oct 1, 2017

Contributor

Hi @shurcooL, could you perhaps tell me why the build is failing? I see it's during the go generate step, but I'm not sure how the code I've written has resulted in a build failure.

Contributor

lackerman commented Oct 1, 2017

Hi @shurcooL, could you perhaps tell me why the build is failing? I see it's during the go generate step, but I'm not sure how the code I've written has resulted in a build failure.

@elliott-beach

This comment has been minimized.

Show comment
Hide comment
@elliott-beach

elliott-beach Oct 1, 2017

Contributor

@lackerman You need to run the command go generate and commit and push, this will generate some accessors for structs.

Contributor

elliott-beach commented Oct 1, 2017

@lackerman You need to run the command go generate and commit and push, this will generate some accessors for structs.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 3, 2017

Member

Nice work on resolving the build failure.

I'm so happy you said that. The flag felt like suck a hack. Not sure I have enough context or experience to comment on the preferred solution, but I'm happy to implement it ;).

Great! The next step here, as far as I can tell, is to factor out that stubbed bool argument from all the new methods.

Looking at it a bit closer, I think we can move it into MarketplaceService struct rather than into the github.Client struct. Perhaps it can look something like this:

// MarketplaceService handles communication with the marketplace related
// methods of the GitHub API.
//
// GitHub API docs: https://developer.github.com/v3/apps/marketplace/
type MarketplaceService struct {
	client *Client

	// Stubbed controls whether endpoints that return stubbed data are used
	// instead of production endpoints. Stubbed data is fake data that's useful
	// for testing your GitHub Apps. Stubbed data is hard-coded and will not
	// change based on actual subscriptions.
	//
	// GitHub API docs: https://developer.github.com/v3/apps/marketplace/
	Stubbed bool
}

Then, users can set it to true if they wish:

client := github.NewClient(nil)
client.MarketplaceService.Stubbed = true
// ...
Member

dmitshur commented Oct 3, 2017

Nice work on resolving the build failure.

I'm so happy you said that. The flag felt like suck a hack. Not sure I have enough context or experience to comment on the preferred solution, but I'm happy to implement it ;).

Great! The next step here, as far as I can tell, is to factor out that stubbed bool argument from all the new methods.

Looking at it a bit closer, I think we can move it into MarketplaceService struct rather than into the github.Client struct. Perhaps it can look something like this:

// MarketplaceService handles communication with the marketplace related
// methods of the GitHub API.
//
// GitHub API docs: https://developer.github.com/v3/apps/marketplace/
type MarketplaceService struct {
	client *Client

	// Stubbed controls whether endpoints that return stubbed data are used
	// instead of production endpoints. Stubbed data is fake data that's useful
	// for testing your GitHub Apps. Stubbed data is hard-coded and will not
	// change based on actual subscriptions.
	//
	// GitHub API docs: https://developer.github.com/v3/apps/marketplace/
	Stubbed bool
}

Then, users can set it to true if they wish:

client := github.NewClient(nil)
client.MarketplaceService.Stubbed = true
// ...
@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Oct 7, 2017

Contributor

Hi @shurcooL what about using a Options wrapper?

// MarketplaceOptions specifies the optional parameters to the Marketplace service methods
type MarketplaceOptions struct {
	// Stubbed controls whether endpoints that return stubbed data are used
	// instead of production endpoints. Stubbed data is fake data that's useful
	// for testing your GitHub Apps. Stubbed data is hard-coded and will not
	// change based on actual subscriptions.
	//
	// GitHub API docs: https://developer.github.com/v3/apps/marketplace/
	Stubbed bool

	ListOptions
}

And then only passing ListOptions to addOptions

uri := marketplaceURI(opt, "plans")
u, err := addOptions(uri, opt.ListOptions)

Or is this pattern specifically used for optional query parameters in the Url?

Contributor

lackerman commented Oct 7, 2017

Hi @shurcooL what about using a Options wrapper?

// MarketplaceOptions specifies the optional parameters to the Marketplace service methods
type MarketplaceOptions struct {
	// Stubbed controls whether endpoints that return stubbed data are used
	// instead of production endpoints. Stubbed data is fake data that's useful
	// for testing your GitHub Apps. Stubbed data is hard-coded and will not
	// change based on actual subscriptions.
	//
	// GitHub API docs: https://developer.github.com/v3/apps/marketplace/
	Stubbed bool

	ListOptions
}

And then only passing ListOptions to addOptions

uri := marketplaceURI(opt, "plans")
u, err := addOptions(uri, opt.ListOptions)

Or is this pattern specifically used for optional query parameters in the Url?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 12, 2017

Member

MarketplaceOptions would work too. So it's just a design decision that's up to us to make.

I think it largely depends on how often one wants to set Stubbed to true. From my current understanding how it's meant to be used, it makes more sense to me to set it once for the entire service while doing testing, rather than for each individual endpoint via the options.

So, unless I'm not understanding the use case properly, I still prefer my client.MarketplaceService.Stubbed = true suggestion above.

@gmlewis @elliott-beach Do you have thoughts/preferences on how to deal with the stubbed parameter?

Member

dmitshur commented Oct 12, 2017

MarketplaceOptions would work too. So it's just a design decision that's up to us to make.

I think it largely depends on how often one wants to set Stubbed to true. From my current understanding how it's meant to be used, it makes more sense to me to set it once for the entire service while doing testing, rather than for each individual endpoint via the options.

So, unless I'm not understanding the use case properly, I still prefer my client.MarketplaceService.Stubbed = true suggestion above.

@gmlewis @elliott-beach Do you have thoughts/preferences on how to deal with the stubbed parameter?

@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Oct 16, 2017

Contributor

Hi @shurcooL, this is probably due to my own ignorance of the language, but when I try your suggested implementation, I get:

# github.com/google/go-github/github
./github.go:231:39: cannot convert &c.common (type *service) to 
type *MarketplaceService

since the Service is of type service by default but now it's being declared as type struct. Is this because I'm missing something that will allow it to implement the service interface?

Contributor

lackerman commented Oct 16, 2017

Hi @shurcooL, this is probably due to my own ignorance of the language, but when I try your suggested implementation, I get:

# github.com/google/go-github/github
./github.go:231:39: cannot convert &c.common (type *service) to 
type *MarketplaceService

since the Service is of type service by default but now it's being declared as type struct. Is this because I'm missing something that will allow it to implement the service interface?

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 16, 2017

Member

@lackerman You wouldn't be able to reuse &c.common struct anymore, because *MarketplaceService will have an extra field. You'll have to create a new *MarketplaceService struct, like so:

-c.Marketplace = (*MarketplaceService)(&c.common)
+c.Marketplace = &MarketplaceService{client: c}
Member

dmitshur commented Oct 16, 2017

@lackerman You wouldn't be able to reuse &c.common struct anymore, because *MarketplaceService will have an extra field. You'll have to create a new *MarketplaceService struct, like so:

-c.Marketplace = (*MarketplaceService)(&c.common)
+c.Marketplace = &MarketplaceService{client: c}
@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Oct 17, 2017

Contributor

Thanks for the help @shurcooL. Please could I ask you to explain the difference between the following two calling conventions:
(*MarketplaceService)(&c.common) and &MarketplaceService{client: c}
The second one makes sense but I'm not seeing how (*XXX)(...) creates a valid instance.

Contributor

lackerman commented Oct 17, 2017

Thanks for the help @shurcooL. Please could I ask you to explain the difference between the following two calling conventions:
(*MarketplaceService)(&c.common) and &MarketplaceService{client: c}
The second one makes sense but I'm not seeing how (*XXX)(...) creates a valid instance.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 17, 2017

Member

@lackerman It works for other services because they're defined as service, e.g.:

type ActivityService service

So *service is convertible to *ActivityService. E.g., see here.

You can find more information about this in #389 and #390 that introduced it. Note, this is a pretty advanced trick, it's not commonly done, and the benefits it offers are nice but not critical.

Member

dmitshur commented Oct 17, 2017

@lackerman It works for other services because they're defined as service, e.g.:

type ActivityService service

So *service is convertible to *ActivityService. E.g., see here.

You can find more information about this in #389 and #390 that introduced it. Note, this is a pretty advanced trick, it's not commonly done, and the benefits it offers are nice but not critical.

@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Oct 17, 2017

Contributor

Ah, a colleague pointed out that it's just a type cast. Due to the syntax (*type)(...), I thought it was some kind or function call/syntactic sugar for new or &.

Contributor

lackerman commented Oct 17, 2017

Ah, a colleague pointed out that it's just a type cast. Due to the syntax (*type)(...), I thought it was some kind or function call/syntactic sugar for new or &.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 17, 2017

Member

Yes, it is. Although if you want to be precise about terms, it's a "type conversion". See https://golang.org/ref/spec#Conversions:

Conversions are expressions of the form T(x) where T is a type and x is an expression that can be converted to type T.

If the type starts with the operator * or <-, or if the type starts with the keyword func and has no result list, it must be parenthesized when necessary to avoid ambiguity

Hence (*T)(x).

Member

dmitshur commented Oct 17, 2017

Yes, it is. Although if you want to be precise about terms, it's a "type conversion". See https://golang.org/ref/spec#Conversions:

Conversions are expressions of the form T(x) where T is a type and x is an expression that can be converted to type T.

If the type starts with the operator * or <-, or if the type starts with the keyword func and has no result list, it must be parenthesized when necessary to avoid ambiguity

Hence (*T)(x).

Added Stubbed tests and switched MarketplaceService to use struct ins鈥
鈥ead of service

Added a flag, `stubbed` to the MarketplaceService struct to set whether or not the Service should called the stubbed endpoints
@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Oct 22, 2017

Contributor

Hi @shurcooL, I've completed the unit tests and performed a basic test to my account on one of the stubbed endpoints and it appears to work as expected.

Contributor

lackerman commented Oct 22, 2017

Hi @shurcooL, I've completed the unit tests and performed a basic test to my account on one of the stubbed endpoints and it appears to work as expected.

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Oct 24, 2017

Member

Good to hear!

I'm not seeing any new commits. Did you mean to push something that implements the changes we discussed above?

Member

dmitshur commented Oct 24, 2017

Good to hear!

I'm not seeing any new commits. Did you mean to push something that implements the changes we discussed above?

@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Oct 24, 2017

Contributor

@shurcooL, sad panda...yeah, I forgot to push. It's pushed now.

Contributor

lackerman commented Oct 24, 2017

@shurcooL, sad panda...yeah, I forgot to push. It's pushed now.

Show outdated Hide outdated github/apps_marketplace.go Outdated
@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Oct 26, 2017

Contributor

@shurcooL fixed the issue you pointed out.

Contributor

lackerman commented Oct 26, 2017

@shurcooL fixed the issue you pointed out.

@gmlewis

This comment has been minimized.

Show comment
Hide comment
@gmlewis

gmlewis Oct 26, 2017

Collaborator

Sorry for the delay - yes, I like the MarketplaceService.Stubbed solution that @shurcooL proposed in the comment above.
I'll now review the PR.

Collaborator

gmlewis commented Oct 26, 2017

Sorry for the delay - yes, I like the MarketplaceService.Stubbed solution that @shurcooL proposed in the comment above.
I'll now review the PR.

@gmlewis

Thank you, @lackerman for doing this! I have just a few things to fix if you don't mind.

Show outdated Hide outdated github/apps_marketplace.go Outdated
// instead of production endpoints. Stubbed data is fake data that's useful
// for testing your GitHub Apps. Stubbed data is hard-coded and will not
// change based on actual subscriptions.
//

This comment has been minimized.

@gmlewis

gmlewis Oct 26, 2017

Collaborator

nit: lines 23-24 are unnecessary due to line 16 above. Please remove them.

@gmlewis

gmlewis Oct 26, 2017

Collaborator

nit: lines 23-24 are unnecessary due to line 16 above. Please remove them.

This comment has been minimized.

@dmitshur

dmitshur Nov 1, 2017

Member

@gmlewis See https://github.com/google/go-github/pull/732/files#r148168072 for why I think it's still helpful to include them. It just so happens the URL matches the one for the service, I wish we could link to a more specific section, but I don't see one...

@dmitshur

dmitshur Nov 1, 2017

Member

@gmlewis See https://github.com/google/go-github/pull/732/files#r148168072 for why I think it's still helpful to include them. It just so happens the URL matches the one for the service, I wish we could link to a more specific section, but I don't see one...

Show outdated Hide outdated github/apps_marketplace.go Outdated
Show outdated Hide outdated github/apps_marketplace.go Outdated
Show outdated Hide outdated github/apps_marketplace.go Outdated
Show outdated Hide outdated github/apps_marketplace.go Outdated
Show outdated Hide outdated github/apps_marketplace.go Outdated
Show outdated Hide outdated github/apps_marketplace_test.go Outdated
})
opt := &ListOptions{Page: 1, PerPage: 2}
client.Marketplace.Stubbed = false

This comment has been minimized.

@gmlewis

gmlewis Oct 26, 2017

Collaborator

Hmmm... I wish we didn't use a package global client for unit testing, as this will prevent unit tests from all being run in parallel in the future.

Ideally, we would have something like:

client, teardown := setup()
defer teardown()

for each test. No action is needed in this PR, but I'll file a new issue and point here for motivation.

@gmlewis

gmlewis Oct 26, 2017

Collaborator

Hmmm... I wish we didn't use a package global client for unit testing, as this will prevent unit tests from all being run in parallel in the future.

Ideally, we would have something like:

client, teardown := setup()
defer teardown()

for each test. No action is needed in this PR, but I'll file a new issue and point here for motivation.

Show outdated Hide outdated github/github.go Outdated
@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Oct 31, 2017

Contributor

Thanks @gmlewis for the extra pair of eyes, really appreciate the suggestions.

Contributor

lackerman commented Oct 31, 2017

Thanks @gmlewis for the extra pair of eyes, really appreciate the suggestions.

lackerman added some commits Oct 31, 2017

Show outdated Hide outdated github/apps_marketplace.go Outdated
Show outdated Hide outdated github/github.go Outdated
@gmlewis

Thank you, @lackerman!
LGTM.
Awaiting second LGTM before merging.

@gmlewis gmlewis requested a review from elliott-beach Oct 31, 2017

Show outdated Hide outdated github/apps_marketplace.go Outdated
@dmitshur

Just one suggested change about Stubbed field documentation, see comment above. Otherwise LGTM.

@gmlewis gmlewis removed the request for review from elliott-beach Nov 1, 2017

@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Nov 12, 2017

Contributor

@shurcooL, updated

Contributor

lackerman commented Nov 12, 2017

@shurcooL, updated

@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Nov 12, 2017

Contributor

@shurcooL not sure why the build failed after adding a comment line

Contributor

lackerman commented Nov 12, 2017

@shurcooL not sure why the build failed after adding a comment line

@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 12, 2017

Member

We have some issues with gen-accessors right now, I've filed #778 for it. But fixing that is out of scope of this PR, so just re-generate the package, and it should be fine.

Member

dmitshur commented Nov 12, 2017

We have some issues with gen-accessors right now, I've filed #778 for it. But fixing that is out of scope of this PR, so just re-generate the package, and it should be fine.

@lackerman

This comment has been minimized.

Show comment
Hide comment
@lackerman

lackerman Nov 12, 2017

Contributor

@shurcooL ready to be merged

Contributor

lackerman commented Nov 12, 2017

@shurcooL ready to be merged

@dmitshur dmitshur merged commit 8b3951c into google:master Nov 14, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dmitshur

This comment has been minimized.

Show comment
Hide comment
@dmitshur

dmitshur Nov 14, 2017

Member

Merged. Thank you @lackerman!

Member

dmitshur commented Nov 14, 2017

Merged. Thank you @lackerman!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment