feat!: Refactor client constructor to use options pattern

[stevehipwell]

BREAKING CHANGE: Clients are now constructed with a nicer builder pattern. See docs for details.

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.75%. Comparing base (a5eca73) to head (5c361f0).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4201      +/-   ##
==========================================
+ Coverage   93.69%   97.75%   +4.05%     
==========================================
  Files         209      189      -20     
  Lines       19772    19035     -737     
==========================================
+ Hits        18526    18608      +82     
+ Misses       1049      231     -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]

@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]

@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]

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!

[stevehipwell]

@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.

[stevehipwell]

@alexandear I'm still waiting on a reply on a couple of threads you opened?

[stevehipwell]

@gmlewis all review comments should now have been addressed.

[gmlewis]

@gmlewis all review comments should now have been addressed.

Thanks, @stevehipwell.
It looks like conflicts must be resolved before we can move forward.

[stevehipwell]

@gmlewis the UI was showing me it could be resolved on the PR so I left it for you to decide. Would you like me to merge master into my branch and push that up?

[gmlewis]

@gmlewis the UI was showing me it could be resolved on the PR so I left it for you to decide. Would you like me to merge master into my branch and push that up?

Yes, please.

[stevehipwell]

@gmlewis done.

[Not-Dhananjay-Mishra]

LGTM 🚀

[gmlewis]

Very nice, @stevehipwell... thank you!
LGTM.

Thank you, @alexandear and @Not-Dhananjay-Mishra!

Merging.