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!

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

	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)

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented May 8, 2026 •

edited

Loading