feat!: Refactor client constructor to use options pattern by stevehipwell · Pull Request #4201 · google/go-github

stevehipwell · 2026-05-08T16:15:22Z

This PR refactors the client to be immutable and to use the with options pattern for construction. This is a significant change but it should reduce the overall complexity and support better UX around new capabilities and backwards compatibility (e.g. github.WithAPIVersion("2026-03-10") or github.WithGHESVersion("3.15")).

Client is immutable
- NewClient is the only way to create a new Client
- After a Client is created if you need to create a new one you can use Clone
- You no longer need to create multiple clients and their associated child structs to chain helpers
Client configuration is explicit
- Options are handled in a specific order to limit the potential for misconfiguration
- Passing invalid values into NewClient causes an early error instead of waiting for a failure or silently doing nothing
Fixed implementation bugs
- clientMu was unnecessary
- Proxy from env pattern lost default transport configuration
Improved test coverage?

Closes #3870
Closes #3915

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

codecov · 2026-05-08T16:33:41Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.77%. Comparing base (6c58592) to head (de722e1).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4201      +/-   ##
==========================================
+ Coverage   93.71%   97.77%   +4.05%     
==========================================
  Files         209      189      -20     
  Lines       19772    19035     -737     
==========================================
+ Hits        18529    18611      +82     
+ Misses       1046      228     -818     
+ Partials      197      196       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gmlewis

Thank you, @stevehipwell.

Although I like the overall pattern, I have a few concerns with this PR.
First, the various fields within Client were originally exported individually, on a case-by-case basis, as users had need to access them. Suddenly unexporting all fields may wreak havoc.

Second, the clientMu was indeed needed to solve a race condition found when copying clients that were currently in-flight, if I'm remembering correctly.

Can we keep the nice builder pattern you've created here without unexporting all the fields and without removing the mutex that is protecting the client?

stevehipwell · 2026-05-08T17:29:40Z

@gmlewis if you look at the code you'll see that clientMu is doing nothing and doesn't even wrap the relevant concerns in the copy function. It used to technically serve a purpose when some of the functions mutated the client, but that no longer happens, although as I said above it wasn't "correct" even for that. I also double checked this with 2 different agents, which both agreed with my summary above.

Unexporting the values and putting them behind the options pattern to make the client immutable is core I the whole pattern. Could you provide some examples where these need changing and Clone is the wrong choice? For example I added the disable rate limit option to the client, it's only exported as there wasn't an existing pattern so that was the simplest approach. I'm happy to add getters for the remaining unexpired fields, but I wasn't sure they were actually needed.

gmlewis · 2026-05-09T15:54:00Z

@gmlewis if you look at the code you'll see that clientMu is doing nothing and doesn't even wrap the relevant concerns in the copy function. It used to technically serve a purpose when some of the functions mutated the client, but that no longer happens, although as I said above it wasn't "correct" even for that. I also double checked this with 2 different agents, which both agreed with my summary above.

Unexporting the values and putting them behind the options pattern to make the client immutable is core I the whole pattern. Could you provide some examples where these need changing and Clone is the wrong choice? For example I added the disable rate limit option to the client, it's only exported as there wasn't an existing pattern so that was the simplest approach. I'm happy to add getters for the remaining unexpired fields, but I wasn't sure they were actually needed.

OK, I see. That sounds good to me.

If you wouldn't mind please cleaning up all the non-idiomatic failures in the examples where I have marked them, that would be greatly appreciated, then we should be ready for a second LGTM+Approval.

gmlewis · 2026-05-09T15:56:28Z

If you could also please investigate the 8 missed lines in code coverage in github/github.go in this PR, that would also be great. Thanks!

alexandear · 2026-05-11T08:08:23Z

-	non-preview functionality, including changes to the exported Go API surface
-	or behavior of the API.
+  non-preview functionality, including changes to the exported Go API surface
+  or behavior of the API.
+
 * We increment the **minor version** with any backwards-compatible changes to
-	functionality, as well as any changes to preview functionality in the GitHub
-	API. GitHub makes no guarantee about the stability of preview functionality,
-	so neither do we consider it a stable part of the go-github API.
+  functionality, as well as any changes to preview functionality in the GitHub
+  API. GitHub makes no guarantee about the stability of preview functionality,
+  so neither do we consider it a stable part of the go-github API.
+


Why were these lines changed? It looks like they are not related to the PR's topic.

I've modified the README and these lines weren't consistently formatted. @gmlewis has asked me to fixup files where I've made changes, so I suggest we keep this for the same reason.

alexandear · 2026-05-11T08:12:26Z

 ```

-*Note*: In order to interact with certain APIs, for example writing a file to a repo, one must generate an installation token
+_Note_: In order to interact with certain APIs, for example writing a file to a repo, one must generate an installation token


Unrelated change:

Suggested change

_Note_: In order to interact with certain APIs, for example writing a file to a repo, one must generate an installation token

*Note*: In order to interact with certain APIs, for example writing a file to a repo, one must generate an installation token

I've modified the README and these lines weren't consistently formatted. @gmlewis has asked me to fixup files where I've made changes, so I suggest we keep this for the same reason.

alexandear · 2026-05-11T08:12:39Z

 [support-policy]: https://golang.org/doc/devel/release.html#policy

-## Development
+## Development ##


Unrelated:

Suggested change

## Development ##

## Development

I've modified the README and these lines weren't consistently formatted. @gmlewis has asked me to fixup files where I've made changes, so I suggest we keep this for the same reason.

alexandear · 2026-05-11T08:22:34Z

-	if c.client == nil {
+// WithSecondaryRateLimitOptions returns a ClientOptionsFunc that configures the Client
+// secondary rate limits.
+func WithSecondaryRateLimitOptions(maxRetryAfterDuration time.Duration) ClientOptionsFunc {


"Options" seems redundant:

Suggested change

func WithSecondaryRateLimitOptions(maxRetryAfterDuration time.Duration) ClientOptionsFunc {

func WithSecondaryRateLimit(maxRetryAfterDuration time.Duration) ClientOptionsFunc {

I've created a single options function here for the secondary rate limit both because I think there may be other options required and because the singular property being set is long. I could rename it to remove the Options suffix, but would that not be confusing given that the secondary rate limit will be enabled even if this isn't passed in?

alexandear · 2026-05-11T09:05:42Z

+	if err != nil {
+		t.Fatal(err)
+	}
+	bobClient, err := clientPreconfiguredWithURLs.Clone(WithAuthToken("bob"))


If we change clientPreconfiguredWithURLs to aliceClient, the test will fail:

Suggested change

bobClient, err := clientPreconfiguredWithURLs.Clone(WithAuthToken("bob"))

bobClient, err := aliceClient.Clone(WithAuthToken("bob"))

=== RUN TestClientCopy_leak_transport === PAUSE TestClientCopy_leak_transport === CONT TestClientCopy_leak_transport /Users/alexandear/src/github.com/google/go-github/github/github_test.go:4291: diff mismatch (-want +got): string( - "Bearer bob", + "Bearer alice", ) --- FAIL: TestClientCopy_leak_transport (0.00s)

But it shouldn't fail because we are overwriting Alice's auth token with Bob's one.

The problem is that Clone(WithAuthToken()) silently uses the original client's token instead of the new one. When cloning a client that already has an auth token set and providing a different token via WithAuthToken, the new token is ignored.

Isn't this exactly the same behaviour as the current version?

stevehipwell · 2026-05-11T10:44:32Z

@gmlewis I've addressed you review comments and should have fixed the test coverage. I've also added exclusions for the examples and other code that shouldn't be be part of the coverage report which should make it easier to see what code should have been tested but hasn't been.

Not-Dhananjay-Mishra · 2026-05-11T16:29:40Z

@@ -118,7 +130,10 @@ func ExamplePullRequestsService_Create() {
 	// github.NewClient() instead of nil. See the following documentation for more


That comment feel misleading since github.NewClient() no longer takes nil. Should we update that?

feat!: refactor client constructor to use options pattern

7e6779b

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

gmlewis changed the title ~~feat!: refactor client constructor to use options pattern~~ feat!: Refactor client constructor to use options pattern May 8, 2026

gmlewis requested changes May 8, 2026

View reviewed changes

gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels May 9, 2026

fixup! feat!: refactor client constructor to use options pattern

4ee0835

alexandear reviewed May 11, 2026

View reviewed changes

Comment thread github/github_test.go Outdated

stevehipwell added 2 commits May 11, 2026 11:15

fixup! feat!: refactor client constructor to use options pattern

b6c4519

fixup! feat!: refactor client constructor to use options pattern

de722e1

Not-Dhananjay-Mishra reviewed May 11, 2026

View reviewed changes

	_Note_: In order to interact with certain APIs, for example writing a file to a repo, one must generate an installation token
	Note: In order to interact with certain APIs, for example writing a file to a repo, one must generate an installation token

	func WithSecondaryRateLimitOptions(maxRetryAfterDuration time.Duration) ClientOptionsFunc {
	func WithSecondaryRateLimit(maxRetryAfterDuration time.Duration) ClientOptionsFunc {

	bobClient, err := clientPreconfiguredWithURLs.Clone(WithAuthToken("bob"))
	bobClient, err := aliceClient.Clone(WithAuthToken("bob"))

		@@ -118,7 +130,10 @@ func ExamplePullRequestsService_Create() {
		// github.NewClient() instead of nil. See the following documentation for more

Conversation

stevehipwell commented May 8, 2026

Uh oh!

codecov Bot commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

gmlewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevehipwell commented May 8, 2026

Uh oh!

gmlewis commented May 9, 2026

Uh oh!

gmlewis commented May 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevehipwell commented May 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented May 8, 2026 •

edited

Loading