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

❌ Patch coverage is 67.16418% with 88 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.48%. Comparing base (6c58592) to head (7e6779b).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
github/github.go	93.04%	8 Missing and 5 partials ⚠️
test/fields/fields.go	0.00%	10 Missing ⚠️
example/otel/main.go	0.00%	7 Missing ⚠️
example/newfilewithappauth/main.go	0.00%	5 Missing ⚠️
example/basicauth/main.go	0.00%	4 Missing ⚠️
example/commitpr/main.go	0.00%	4 Missing ⚠️
example/contents/main.go	0.00%	4 Missing ⚠️
example/ratelimit/main.go	0.00%	4 Missing ⚠️
example/actionpermissions/main.go	0.00%	3 Missing ⚠️
...xample/codespaces/newreposecretwithxcrypto/main.go	0.00%	3 Missing ⚠️
... and 11 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4201      +/-   ##
==========================================
- Coverage   93.71%   93.48%   -0.23%     
==========================================
  Files         209      209              
  Lines       19772    19885     +113     
==========================================
+ Hits        18529    18590      +61     
- Misses       1046     1094      +48     
- Partials      197      201       +4

☔ 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?

gmlewis · 2026-05-08T16:27:04Z

+		fmt.Printf("\nerror: %v\n", err)
+		return


Suggested change

fmt.Printf("\nerror: %v\n", err)

return

log.Fatalf("error: %v", err)

I intentionally kept the existing pattern for the individual examples rather than standardising them all.

No, let's please clean them all up and standardize them since you are touching every one of these in this PR.

gmlewis · 2026-05-08T16:28:28Z

+		fmt.Printf("Error creating GitHub client: %v\n", err)
+		os.Exit(1)


Suggested change

fmt.Printf("Error creating GitHub client: %v\n", err)

os.Exit(1)

log.Fatalf("Error creating GitHub client: %v", err)

gmlewis · 2026-05-08T16:31:04Z

+		fmt.Printf("Error creating GitHub client: %v\n", err)
+		return


Suggested change

fmt.Printf("Error creating GitHub client: %v\n", err)

return

log.Fatalf("Error creating GitHub client: %v", err)

gmlewis · 2026-05-08T16:31:39Z

-	client := github.NewClient(nil).WithAuthToken(string(token))
+	client, err := github.NewClient(github.WithAuthToken(string(token)))
+	if err != nil {
+		log.Fatalf("Error creating GitHub client: %v\n", err)


Suggested change

log.Fatalf("Error creating GitHub client: %v\n", err)

log.Fatalf("Error creating GitHub client: %v", err)

gmlewis · 2026-05-08T16:31:58Z

-	client := github.NewClient(nil).WithAuthToken(token)
+	client, err := github.NewClient(github.WithAuthToken(token))
+	if err != nil {
+		log.Fatalf("Error creating GitHub client: %v\n", err)


Suggested change

log.Fatalf("Error creating GitHub client: %v\n", err)

log.Fatalf("Error creating GitHub client: %v", err)

gmlewis · 2026-05-08T16:36:57Z

+			fmt.Printf("Error creating GitHub client: %v\n", err)
+			os.Exit(1)


Suggested change

fmt.Printf("Error creating GitHub client: %v\n", err)

os.Exit(1)

log.Fatalf("Error creating GitHub client: %v", err)

gmlewis · 2026-05-08T16:37:21Z

+			fmt.Printf("Error creating GitHub client with token: %v\n", err)
+			os.Exit(1)


Suggested change

fmt.Printf("Error creating GitHub client with token: %v\n", err)

os.Exit(1)

log.Fatalf("Error creating GitHub client with token: %v", err)

gmlewis · 2026-05-08T16:38:00Z

+			fmt.Printf("Error creating GitHub client: %v\n", err)
+			os.Exit(1)


Suggested change

fmt.Printf("Error creating GitHub client: %v\n", err)

os.Exit(1)

log.Fatalf("Error creating GitHub client: %v", err)

gmlewis · 2026-05-08T16:38:19Z

+		fmt.Printf("Error creating GitHub client with token: %v\n", err)
+		os.Exit(1)


Suggested change

fmt.Printf("Error creating GitHub client with token: %v\n", err)

os.Exit(1)

log.Fatalf("Error creating GitHub client with token: %v", err)

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-11T08:56:15Z

+// Note: When using a nil httpClient, the default client has no timeout set.
+// This may not be suitable for production environments. It is recommended to
+// provide a custom http.Client with an appropriate timeout.


NewClient no longer accepts an http.Client argument or nil http.Client. This note is now misleading and should be rewritten.

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.

alexandear · 2026-05-11T09:08:44Z

+		}
+
+		if opts.httpClient == nil {
+			t.Error("httpClient is nil")


Suggested change

t.Error("httpClient is nil")

t.Fatal("httpClient is nil")

alexandear · 2026-05-11T09:09:04Z

+		opts := clientOptions{}
+		err := WithHTTPClient(customClient)(&opts)
+		if err != nil {
+			t.Errorf("WithHTTPClient errored: %v", err)


Suggested change

t.Errorf("WithHTTPClient errored: %v", err)

t.Fatalf("WithHTTPClient errored: %v", err)

alexandear · 2026-05-11T09:09:33Z

+		opts := clientOptions{}
+		err := WithUserAgent("")(&opts)
+		if err != nil {
+			t.Errorf("WithUserAgent errored: %v", err)


Suggested change

t.Errorf("WithUserAgent errored: %v", err)

t.Fatalf("WithUserAgent errored: %v", err)

alexandear · 2026-05-11T09:13:37Z

+			opts := clientOptions{}
+			err := WithSecondaryRateLimitOptions(tt.maxRetryAfterDuration)(&opts)
+			if err != nil {
+				t.Errorf("WithSecondaryRateLimitOptions errored: %v", err)


Suggested change

t.Errorf("WithSecondaryRateLimitOptions errored: %v", err)

t.Fatalf("WithSecondaryRateLimitOptions errored: %v", err)

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

	fmt.Printf("\nerror: %v\n", err)
	return
	log.Fatalf("error: %v", err)

		fmt.Printf("Error creating GitHub client: %v\n", err)
		os.Exit(1)

	fmt.Printf("Error creating GitHub client: %v\n", err)
	os.Exit(1)
	log.Fatalf("Error creating GitHub client: %v", err)

	_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"))

	t.Errorf("WithHTTPClient errored: %v", err)
	t.Fatalf("WithHTTPClient errored: %v", err)

	t.Errorf("WithUserAgent errored: %v", err)
	t.Fatalf("WithUserAgent errored: %v", err)

	t.Errorf("WithSecondaryRateLimitOptions errored: %v", err)
	t.Fatalf("WithSecondaryRateLimitOptions errored: %v", err)

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!

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!

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented May 8, 2026 •

edited

Loading