Add interfaces to embed with some changes #1
Conversation
Codecov Report
@@ Coverage Diff @@
## add-interfaces-to-embed #1 +/- ##
==========================================================
Coverage ? 97.16%
==========================================================
Files ? 105
Lines ? 6662
Branches ? 0
==========================================================
Hits ? 6473
Misses ? 115
Partials ? 74 Continue to review full report at Codecov.
|
| Interactions InteractionsServiceInterface | ||
| IssueImport IssueImportServiceInterface | ||
| Issues IssuesServiceInterface | ||
| Licenses LicensesServiceInterface | ||
| Marketplace *MarketplaceService |
There was a problem hiding this comment.
Need to rework https://github.com/gmlewis/go-github/blob/master/github/apps_marketplace_test.go#L62 before changing this to also use the interface.
|
@migueleliasweb - thank you for your modified example. Obviously, this could also have been achieved without an embedded interface... However, I suppose there is the one big advantage that you can pass around a (Note that your PR needs some changes to make it idiomatic Go, but we can hold off on the code review until we hear from Will.) @willnorris - please take a look and comment when you have time. |
|
Hey @willnorris, could you please have a look at this? Just so this idea doesn't die out. I just wanted to know your thoughts and if we can move forward this this idea. |
|
Personally, I still don't love the addition of all these interfaces to the main github package. The package docs are already pretty overwhelming as-is, and this just makes that worse, and adds an extra layer of indirection making code harder to read. At least with how others have done it (mentioned here) the interfaces are in a separate package and are only brought in by users who care about them. I think something like that would be my preference. That said, I'm not sure that I should necessarily have the final say here. I don't actually use this library myself anymore (though I guess I'm not sure how much @gmlewis does either 😄). |
That's an interesting idea... would moving this to its own package (
😄 In 2020 I was using this repo a lot, but you are right, Will... not much these days. |
|
/cc @gauntface @taquitos @rspier in case they want to weigh in on design (though this discussion is currently spread across two PRs and an issue at this point). But I know this is an issue Google has run into as well in order to test code that uses go-github. |
Since the interfaces are used in the main struct Eg: githubiface, ghiface, iface (as we're already in the go-github repo?), sound reasonable. Ps: @gmlewis if you want to merge this PR onto yours and make further changes to whatever code I wrote, feel free. This was only me trying to show what I had in mind and how I would use the interfaces in production code. |
|
I commented on google#1800 about concerns about causing production issues with the overall approach, but I can't deny there is a lot of upside to using interfaces like this. It would definitely simplify testing and overriding. I'm also trying to square this overall change against https://github.com/golang/go/wiki/CodeReviewComments#interfaces -- I've mostly convinced myself it doesn't violate it. Will's concern about the documentation is an important one. Can we get pkg.go.dev to preview it so we can see what the options look like? |
As far as I can tell, the answer is "no" unless we cut a release of it in the main repo, then later revert the changes and cut a new release. I tried cutting a release within my branch on my forked repo, and pkg.go.dev refuses to display it, seeing that the I suppose I could globally replace "google" => "gmlewis" but that just seems like a bad idea. Honestly, the auto-generated docs for this repo are already insanely large... I'm surprised the browser didn't choke. I think it would be awesome to split each "Service" into its own documentation page (by making them separate subdirs/packages)... but I'm thinking that would be a seriously breaking API change that would anger more people than it would please. |
Thank you for this link, @rspier ! This was my understanding as well... I'm glad you found the official statement. I'm still having a difficult time justifying putting this in the same package... it almost belongs in a "mock" package, I think. Let me try that out for size. |
Ah! I now see the challenge... I just spent an hour moving the interfaces to its own "mock" package and then started modifying the example test case to use it. For some reason, the big picture didn't click before, but now I see that this is not possible without changing each service pointer in the main repo to the mocked interface! So now I'm extremely conflicted and thinking that this is just not a good idea all around. |
I think this is a worthy long-term goal, but out of-scope/orthogonal to the current discussion. It may be possible to provide a transparent transition layer for most people relying on the current layout. |
If we move forward with this, we can generate the pages locally, maybe upload as an attachment or shove somewhere so people can see the changes. The question is really "does the change make the already difficult to read page significantly harder to read?" https://github.com/marketplace/actions/godoc-action is the kind of thing I was thinking of for staging changes (This particular one is implemented as a short shell script, interpret that as you will.) |
Comparing against the master branch, at least in text form, it's just: --- /tmp/before 2021-04-02 09:10:04.068469733 -0700
+++ /tmp/after 2021-04-02 09:09:53.056357217 -0700
@@ -209,10 +209,13 @@
type ActionsEnabledOnOrgRepos struct{ ... }
type ActionsPermissions struct{ ... }
type ActionsService service
+type ActionsServiceInterface interface{ ... }
type ActivityListStarredOptions struct{ ... }
type ActivityService service
+type ActivityServiceInterface interface{ ... }
type AdminEnforcement struct{ ... }
type AdminService service
+type AdminServiceInterface interface{ ... }
type AdminStats struct{ ... }
type Alert struct{ ... }
type AlertListOptions struct{ ... }
@@ -221,6 +224,7 @@
type App struct{ ... }
type AppConfig struct{ ... }
type AppsService service
+type AppsServiceInterface interface{ ... }
type ArchiveFormat string
const Tarball ArchiveFormat = "tarball" ...
type Artifact struct{ ... }
@@ -232,9 +236,11 @@
type AuthorizationRequest struct{ ... }
type AuthorizationUpdateRequest struct{ ... }
type AuthorizationsService service
+type AuthorizationsServiceInterface interface{ ... }
type AutoTriggerCheck struct{ ... }
type BasicAuthTransport struct{ ... }
type BillingService service
+type BillingServiceInterface interface{ ... }
type Blob struct{ ... }
type Branch struct{ ... }
type BranchCommit struct{ ... }
@@ -252,12 +258,14 @@
type CheckSuitePreferenceOptions struct{ ... }
type CheckSuitePreferenceResults struct{ ... }
type ChecksService service
+type ChecksServiceInterface interface{ ... }
type Client struct{ ... }
func NewClient(httpClient *http.Client) *Client
func NewEnterpriseClient(baseURL, uploadURL string, httpClient *http.Client) (*Client, error)
type CodeOfConduct struct{ ... }
type CodeResult struct{ ... }
type CodeScanningService service
+type CodeScanningServiceInterface interface{ ... }
type CodeSearchResult struct{ ... }
type CollaboratorInvitation struct{ ... }
type CombinedStatus struct{ ... }
@@ -306,6 +314,7 @@
type EncryptedSecret struct{ ... }
type Enterprise struct{ ... }
type EnterpriseService service
+type EnterpriseServiceInterface interface{ ... }
type Error struct{ ... }
type ErrorResponse struct{ ... }
type Event struct{ ... }
@@ -324,11 +333,14 @@
type GistListOptions struct{ ... }
type GistStats struct{ ... }
type GistsService service
+type GistsServiceInterface interface{ ... }
type GitHubAppAuthorizationEvent struct{ ... }
type GitObject struct{ ... }
type GitService service
+type GitServiceInterface interface{ ... }
type Gitignore struct{ ... }
type GitignoresService service
+type GitignoresServiceInterface interface{ ... }
type GollumEvent struct{ ... }
type Grant struct{ ... }
type HeadCommit struct{ ... }
@@ -349,6 +361,7 @@
type InstallationTokenOptions struct{ ... }
type InteractionRestriction struct{ ... }
type InteractionsService service
+type InteractionsServiceInterface interface{ ... }
type Invitation struct{ ... }
type Issue struct{ ... }
type IssueComment struct{ ... }
@@ -359,6 +372,7 @@
type IssueImportRequest struct{ ... }
type IssueImportResponse struct{ ... }
type IssueImportService service
+type IssueImportServiceInterface interface{ ... }
type IssueListByRepoOptions struct{ ... }
type IssueListCommentsOptions struct{ ... }
type IssueListOptions struct{ ... }
@@ -367,6 +381,7 @@
type IssuesEvent struct{ ... }
type IssuesSearchResult struct{ ... }
type IssuesService service
+type IssuesServiceInterface interface{ ... }
type Jobs struct{ ... }
type Key struct{ ... }
type Label struct{ ... }
@@ -376,6 +391,7 @@
type LargeFile struct{ ... }
type License struct{ ... }
type LicensesService service
+type LicensesServiceInterface interface{ ... }
type ListCheckRunsOptions struct{ ... }
type ListCheckRunsResults struct{ ... }
type ListCheckSuiteOptions struct{ ... }
@@ -400,6 +416,7 @@
type MarketplacePurchase struct{ ... }
type MarketplacePurchaseEvent struct{ ... }
type MarketplaceService struct{ ... }
+type MarketplaceServiceInterface interface{ ... }
type Match struct{ ... }
type MemberEvent struct{ ... }
type Membership struct{ ... }
@@ -409,6 +426,7 @@
type Migration struct{ ... }
type MigrationOptions struct{ ... }
type MigrationService service
+type MigrationServiceInterface interface{ ... }
type Milestone struct{ ... }
type MilestoneEvent struct{ ... }
type MilestoneListOptions struct{ ... }
@@ -427,6 +445,7 @@
type OrganizationInstallations struct{ ... }
type OrganizationsListOptions struct{ ... }
type OrganizationsService service
+type OrganizationsServiceInterface interface{ ... }
type PRLink struct{ ... }
type PRLinks struct{ ... }
type Package struct{ ... }
@@ -467,6 +486,7 @@
type ProjectOptions struct{ ... }
type ProjectPermissionLevel struct{ ... }
type ProjectsService service
+type ProjectsServiceInterface interface{ ... }
type Protection struct{ ... }
type ProtectionRequest struct{ ... }
type PublicEvent struct{ ... }
@@ -491,6 +511,7 @@
type PullRequestReviewsEnforcementRequest struct{ ... }
type PullRequestReviewsEnforcementUpdate struct{ ... }
type PullRequestsService service
+type PullRequestsServiceInterface interface{ ... }
type PullStats struct{ ... }
type PunchCard struct{ ... }
type PushEvent struct{ ... }
@@ -505,6 +526,7 @@
type Reaction struct{ ... }
type Reactions struct{ ... }
type ReactionsService service
+type ReactionsServiceInterface interface{ ... }
type Reference struct{ ... }
type ReferenceListOptions struct{ ... }
type RegistrationToken struct{ ... }
@@ -517,6 +539,7 @@
type RepoStatus struct{ ... }
type RepositoriesSearchResult struct{ ... }
type RepositoriesService service
+type RepositoriesServiceInterface interface{ ... }
type Repository struct{ ... }
type RepositoryAddCollaboratorOptions struct{ ... }
type RepositoryComment struct{ ... }
@@ -557,6 +580,7 @@
const ScopeNone Scope = "(no scope)" ...
type SearchOptions struct{ ... }
type SearchService service
+type SearchServiceInterface interface{ ... }
type Secret struct{ ... }
type Secrets struct{ ... }
type SelectedRepoIDs []int64
@@ -587,6 +611,7 @@
type TeamListTeamMembersOptions struct{ ... }
type TeamProjectOptions struct{ ... }
type TeamsService service
+type TeamsServiceInterface interface{ ... }
type TemplateRepoRequest struct{ ... }
type TextMatch struct{ ... }
type Timeline struct{ ... }
@@ -620,6 +645,7 @@
type UserSuspendOptions struct{ ... }
type UsersSearchResult struct{ ... }
type UsersService service
+type UsersServiceInterface interface{ ... }
type WatchEvent struct{ ... }
type WebHookAuthor struct{ ... }
type WebHookCommit struct{ ... } |
|
If we need an educated outside opinion, we could ask our acquaintances on the go-team what they think. It's a little hard as the changes are spread across 3 PRs and issues, but I've found them a good resource for this kind of thing. |
|
Regarding the docs issue, I think I've come to the conclusion that "it is what it is". The page goes from very very very long (674 US-Letter pages) to very very very longer (781 US-Letter Pages). The addition is new sections that look like this:
(This is a short one to make the screenshot easier.) |
So 100*(781-674)/674 = 16% increase in size. Ouch. I could remove all the comments. Should we take a look at that version? |
It's big in relative terms, but in absolute terms it doesn't make a big difference. It's big and slow before, and big and slow after.
Why not? :) You could leave one comment pointing to the appropriate Service, since it's going to be identical. |
Sounds good. I made this change in my original PR and updated my example to use the new mechanism. @rspier - @migueleliasweb - I know this is getting confusing, so I'm going to close this PR because I updated my original PR here: google#1832 Let's please continue discussion there and not here. |
I've added some required changes to
github.Clientproperties. Now they're using the interface type that allows those properties to be easily overridable for unittests. Also added a more exhaustive unittest example that shows how I initially envisioned the usage of the interfaces.I also found one problem with
github.Client.Marketplace. There's a test case ingithub/apps_marketplace_test.gothat accesses an internal property like soclient.Marketplace.Stubbed = true. This won't work when we start using the interfaces for the property types ingithub.Client. I left it out for now. Need to rework the test to not use that property somehow.Ping @gmlewis . Please have a look at this.