feat!: refactor client constructor to use options pattern#23
Open
alexandear wants to merge 2 commits into
Open
feat!: refactor client constructor to use options pattern#23alexandear wants to merge 2 commits into
alexandear wants to merge 2 commits into
Conversation
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis pull request refactors the go-github client construction API from a chainable builder pattern to a functional options pattern. ChangesClient API Core Refactor
Test Suite
Service-Level Updates
Documentation & Examples
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the github.Client constructor to a functional options pattern (github.NewClient(opts ...ClientOptionsFunc) (*Client, error)), updates internal configuration fields, and propagates the new API through tests, tooling, docs, and examples across the repository.
Changes:
- Replaced the old
NewClient(httpClient *http.Client) *Client+ chaining methods (e.g.,.WithAuthToken) withNewClient(...options)and option helpers likeWithAuthToken,WithHTTPClient,WithTransport,WithEnterpriseURLs. - Made several
Clientconfiguration fields unexported (e.g., base/upload URL, user agent, rate-limit toggles) and adjusted package code/tests accordingly. - Updated README, package docs, integration tests, and many examples to use the new constructor and handle errors.
Reviewed changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/metadata/main.go | Updates internal tooling to create a client via the new options-based constructor (including enterprise base + upload URL). |
| test/integration/github_test.go | Migrates integration client initialization to new constructor and token option. |
| test/integration/authorizations_test.go | Updates OAuth app client creation to use WithHTTPClient and handle constructor errors. |
| test/fields/fields.go | Updates the standalone fields test harness to use the new constructor and token option. |
| README.md | Updates documentation snippets to the new NewClient() API and options usage. |
| github/repos_releases_test.go | Adjusts tests to use the updated base URL field access. |
| github/repos_contents.go | Switches to the new internal redirection/rate-limit configuration field. |
| github/repos_contents_test.go | Updates tests to set the new internal redirection/rate-limit configuration field. |
| github/rate_limit.go | Switches to the new internal disable-rate-limit-check field. |
| github/rate_limit_test.go | Updates tests to set the new internal disable-rate-limit-check field. |
| github/github.go | Core refactor: introduces options types, new constructor, new clone path, internalizes configuration fields, and adds accessors. |
| github/github_test.go | Reworks/expands tests to validate option functions, new constructor behavior, and cloning behavior. |
| github/examples_test.go | Updates godoc examples to use the error-returning constructor. |
| github/example_iterators_test.go | Updates iterator example to use the error-returning constructor. |
| github/doc.go | Updates package-level docs to describe and demonstrate the new options-based constructor/auth. |
| github/copilot_test.go | Adjusts tests that build absolute URLs to use the updated base URL access. |
| github/actions_workflows_test.go | Updates tests that intentionally break BaseURL to use the updated internal base URL field. |
| github/actions_workflow_runs.go | Switches to the new internal redirection/rate-limit configuration field. |
| github/actions_workflow_runs_test.go | Updates tests to set the new internal redirection/rate-limit configuration field. |
| github/actions_workflow_jobs.go | Switches to the new internal redirection/rate-limit configuration field. |
| github/actions_workflow_jobs_test.go | Updates tests for new constructor behavior and uses Clone+WithTransport for “bad transport” cases. |
| github/actions_artifacts.go | Switches to the new internal redirection/rate-limit configuration field. |
| github/actions_artifacts_test.go | Updates tests for new constructor behavior and uses Clone+WithTransport for “bad transport” cases. |
| example/verifyartifact/main.go | Updates example to use NewClient(WithAuthToken(...)) and handle errors. |
| example/uploadreleaseassetfromrelease/main.go | Updates example to use NewClient(WithAuthToken(...)) and handle errors. |
| example/topics/main.go | Updates example to use NewClient() and switches error handling to fatal logging. |
| example/tokenauth/main.go | Updates example to use NewClient(WithAuthToken(...)) and handle errors. |
| example/simple/main.go | Updates example to use NewClient() and switches error handling to fatal logging. |
| example/ratelimit/main.go | Updates example to use WithHTTPClient option for the paginator client. |
| example/otel/main.go | Updates example to use WithTransport option for OTel transport. |
| example/newreposecretwithxcrypto/main.go | Updates example to use NewClient(WithAuthToken(...)) and handle errors. |
| example/newrepo/main.go | Updates example to use NewClient(WithAuthToken(...)) and handle errors. |
| example/newfilewithappauth/main.go | Updates example to compose options (WithHTTPClient, WithEnterpriseURLs, WithAuthToken). |
| example/migrations/main.go | Updates example to use NewClient(WithAuthToken(...)) and switches error handling to fatal logging. |
| example/listenvironments/main.go | Updates example to use NewClient(WithAuthToken(...)) and handle errors. |
| example/contents/main.go | Updates example to use NewClient() and switches error handling to fatal logging. |
| example/commitpr/main.go | Updates example to use NewClient(WithAuthToken(...)) and handle errors. |
| example/codespaces/newusersecretwithxcrypto/main.go | Updates example to use NewClient(WithAuthToken(...)) and handle errors. |
| example/codespaces/newreposecretwithxcrypto/main.go | Updates example to use NewClient(WithAuthToken(...)) and handle errors. |
| example/basicauth/main.go | Updates example to use WithHTTPClient option and switches error handling to fatal logging. |
| example/auditlogstream/main.go | Updates example to construct client via options (WithAuthToken, WithEnterpriseURLs). |
| example/actionpermissions/main.go | Updates example to use NewClient(WithAuthToken(...)) and handle errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
github/actions_workflows_test.go (1)
266-270:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale assertion text to match the new internal field name.
After switching to
client.baseURL.Path(Line 266, Line 321, Line 414, Line 449, Line 484, Line 519), the failure messages still referenceclient.BaseURL.Path. Please update those strings to keep test failures accurate and less confusing.Suggested patch
- t.Error("client.BaseURL.Path='' CreateWorkflowDispatchEventByID err = nil, want error") + t.Error("client.baseURL.Path='' CreateWorkflowDispatchEventByID err = nil, want error") - t.Error("client.BaseURL.Path='' CreateWorkflowDispatchEventByFileName err = nil, want error") + t.Error("client.baseURL.Path='' CreateWorkflowDispatchEventByFileName err = nil, want error") - t.Error("client.BaseURL.Path='' EnableWorkflowByID err = nil, want error") + t.Error("client.baseURL.Path='' EnableWorkflowByID err = nil, want error") - t.Error("client.BaseURL.Path='' EnableWorkflowByFilename err = nil, want error") + t.Error("client.baseURL.Path='' EnableWorkflowByFilename err = nil, want error") - t.Error("client.BaseURL.Path='' DisableWorkflowByID err = nil, want error") + t.Error("client.baseURL.Path='' DisableWorkflowByID err = nil, want error") - t.Error("client.BaseURL.Path='' DisableWorkflowByFileName err = nil, want error") + t.Error("client.baseURL.Path='' DisableWorkflowByFileName err = nil, want error")Also applies to: 321-325, 414-418, 449-453, 484-488, 519-523
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@github/actions_workflows_test.go` around lines 266 - 270, Update the stale assertion messages in the tests that still reference client.BaseURL.Path to use the new internal field name client.baseURL.Path; locate the failing t.Error/t.Fatalf messages (e.g., the assertion after calling client.Actions.CreateWorkflowDispatchEventByID and the other similar assertions in the same test file) and change the string "client.BaseURL.Path=''" (and any other occurrences of "client.BaseURL.Path") to "client.baseURL.Path=''" so failure output matches the actual field name used in the code.github/examples_test.go (1)
78-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale auth guidance comments to the new constructor style.
On Lines 80, 130, and 160, comments still describe providing an OAuth client “instead of nil,” which no longer matches the
NewClient(opts...)API and can mislead readers.✏️ Suggested comment update
- // github.NewClient() instead of nil. See the following documentation for more + // github.NewClient(github.WithAuthToken("<token>")) (or another auth option). + // See the following documentation for moreApply the same wording update in the three affected comment blocks.
Also applies to: 128-132, 158-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@github/examples_test.go` around lines 78 - 82, Update the outdated comment wording that tells readers to pass an OAuth client "instead of nil" to github.NewClient(); change all three comment blocks in examples_test.go so they reference the current constructor style github.NewClient(opts...) and instruct the reader to supply an authenticated option (e.g., an oauth2 authenticated http.Client wrapped as an option) rather than passing nil; apply the same replacement to each occurrence that currently mentions "instead of nil" so the guidance matches the NewClient(opts...) API.
🧹 Nitpick comments (3)
github/github.go (1)
886-886: ⚡ Quick winInconsistent User-Agent handling vs.
NewRequest/NewFormRequest.
NewRequest(line 776) andNewFormRequest(line 813) guard withif c.userAgent != ""before setting the header, which means a deliberately emptyWithUserAgent("")results in noUser-Agentheader.NewUploadRequestunconditionally callsreq.Header.Set("User-Agent", c.userAgent), so the same configuration yields a header with an empty value here. Either suppress in all three, or set in all three — current state is inconsistent.♻️ Suggested fix
req.Header.Set("Content-Type", mediaType) req.Header.Set("Accept", mediaTypeV3) - req.Header.Set("User-Agent", c.userAgent) + if c.userAgent != "" { + req.Header.Set("User-Agent", c.userAgent) + } req.Header.Set(headerAPIVersion, defaultAPIVersion)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@github/github.go` at line 886, NewUploadRequest currently unconditionally sets req.Header.Set("User-Agent", c.userAgent) which is inconsistent with NewRequest and NewFormRequest (they only set the header when c.userAgent != ""), so update NewUploadRequest to match by only setting the "User-Agent" header when c.userAgent != "" (i.e., add the same guard used in NewRequest/NewFormRequest) so a deliberate WithUserAgent("") results in no header across all request constructors.github/actions_artifacts_test.go (1)
294-294: Direct internal field access in tests is acceptable but couples to implementation.Setting
client.rateLimitRedirectionalEndpointsdirectly works for same-package tests, but if this field is frequently needed in tests, consider adding aWithRateLimitRedirectionalEndpointsoption for better encapsulation and forward compatibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@github/actions_artifacts_test.go` at line 294, Tests are directly mutating the internal field rateLimitRedirectionalEndpoints on the client, which couples tests to implementation; add a constructor option like WithRateLimitRedirectionalEndpoints(bool) and accept it in the NewClient or client creation flow so tests can set this behavior via the public API instead of assigning client.rateLimitRedirectionalEndpoints directly; update tests to use WithRateLimitRedirectionalEndpoints(tc.respectRateLimits) and adjust the client/options handling code to apply that option during initialization.github/github_test.go (1)
1199-1213: ⚡ Quick winClarify misleading "as original" error messages in Clone subtests.
In
initialized_client_with_transport(Line 1202–1203) andinitialized_client_with_http_client_and_transport(Line 1276–1277), the assertion comparescloned.client.Transport != tr, but the error message says "is not the same instance as original". The check is actually validating that the cloned client uses the transport supplied viaWithTransport(tr), not the original transport onc. Same pattern appears at Line 1238–1239 (!= h.Transport). Aligning the messages with what is being checked will make failures easier to debug.📝 Suggested clarifications
@@ initialized_client_with_transport if cloned.client.Transport != tr { - t.Error("Cloned Client http.Client.Transport is not the same instance as original") + t.Error("Cloned Client http.Client.Transport is not the same instance as provided via WithTransport") } @@ initialized_client_with_http_client if cloned.client.Transport != h.Transport { - t.Error("Cloned Client http.Client.Transport is not the same instance as original") + t.Error("Cloned Client http.Client.Transport is not the same instance as the WithHTTPClient transport") } @@ initialized_client_with_http_client_and_transport if cloned.client.Transport != tr { - t.Error("Cloned Client http.Client.Transport is not the same instance as original") + t.Error("Cloned Client http.Client.Transport is not the same instance as provided via WithTransport") }Also applies to: 1273-1287
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@github/github_test.go` around lines 1199 - 1213, The test error messages are misleading: the checks using cloned.client.Transport != tr (and cloned.client.Transport != h.Transport) are verifying that Clone used the transport passed via WithTransport(tr)/WithTransport(h.Transport), not that it kept the "original" transport from c; update the failure strings to say "does not equal supplied transport (tr)" or "does not equal supplied transport (h.Transport)" respectively in the initialized_client_with_transport and initialized_client_with_http_client_and_transport subtests (and the analogous check comparing to h.Transport), keeping references to cloned.client.Transport, tr, h.Transport and WithTransport to locate the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@example/migrations/main.go`:
- Line 21: Replace the hardcoded placeholder token in the github client creation
with the token read from the environment; change the call that creates the
client (the line that constructs client via github.NewClient(...)) to use
os.Getenv("GITHUB_AUTH_TOKEN") and guard/handle the case when the env var is
empty (e.g., log and exit or return an error) so the example aligns with other
code that sources the token from the environment.
In `@github/github_test.go`:
- Around line 466-484: Change the nil-check in the test that uses WithHTTPClient
so it fails immediately instead of continuing: replace the t.Error call that
asserts opts.httpClient == nil with t.Fatal (or t.Fatalf) so the test aborts
before the subsequent dereference (opts.httpClient.Timeout); this reference
occurs in the clientOptions test where opts is set by WithHTTPClient and
prevents a nil-pointer panic when httpClient is unexpectedly nil.
- Around line 780-790: The table-driven test dereferences
opts.maxSecondaryRateLimitRetryAfterDuration without checking for nil; update
the subtest in github_test.go to guard before dereferencing (when calling
WithSecondaryRateLimitOptions and asserting the value) by first verifying
opts.maxSecondaryRateLimitRetryAfterDuration != nil and failing the test with
t.Fatalf or using a separate assertion if it is nil, otherwise compare
*opts.maxSecondaryRateLimitRetryAfterDuration to tt.maxRetryAfterDuration;
reference clientOptions, WithSecondaryRateLimitOptions and the
opts.maxSecondaryRateLimitRetryAfterDuration field when making the change.
- Around line 523-535: In the "empty_user_agent" subtest, avoid dereferencing
opts.userAgent without a nil check and fix the error message to print the string
value; update the test to first assert opts.userAgent != nil (or handle nil as a
failure) before accessing *opts.userAgent and change the t.Errorf placeholder to
use the dereferenced string (e.g., "%q" or "%s" with *opts.userAgent) so
failures show the actual user-agent value; reference the clientOptions type and
the WithUserAgent function when making the guard and message change.
---
Outside diff comments:
In `@github/actions_workflows_test.go`:
- Around line 266-270: Update the stale assertion messages in the tests that
still reference client.BaseURL.Path to use the new internal field name
client.baseURL.Path; locate the failing t.Error/t.Fatalf messages (e.g., the
assertion after calling client.Actions.CreateWorkflowDispatchEventByID and the
other similar assertions in the same test file) and change the string
"client.BaseURL.Path=''" (and any other occurrences of "client.BaseURL.Path") to
"client.baseURL.Path=''" so failure output matches the actual field name used in
the code.
In `@github/examples_test.go`:
- Around line 78-82: Update the outdated comment wording that tells readers to
pass an OAuth client "instead of nil" to github.NewClient(); change all three
comment blocks in examples_test.go so they reference the current constructor
style github.NewClient(opts...) and instruct the reader to supply an
authenticated option (e.g., an oauth2 authenticated http.Client wrapped as an
option) rather than passing nil; apply the same replacement to each occurrence
that currently mentions "instead of nil" so the guidance matches the
NewClient(opts...) API.
---
Nitpick comments:
In `@github/actions_artifacts_test.go`:
- Line 294: Tests are directly mutating the internal field
rateLimitRedirectionalEndpoints on the client, which couples tests to
implementation; add a constructor option like
WithRateLimitRedirectionalEndpoints(bool) and accept it in the NewClient or
client creation flow so tests can set this behavior via the public API instead
of assigning client.rateLimitRedirectionalEndpoints directly; update tests to
use WithRateLimitRedirectionalEndpoints(tc.respectRateLimits) and adjust the
client/options handling code to apply that option during initialization.
In `@github/github_test.go`:
- Around line 1199-1213: The test error messages are misleading: the checks
using cloned.client.Transport != tr (and cloned.client.Transport != h.Transport)
are verifying that Clone used the transport passed via
WithTransport(tr)/WithTransport(h.Transport), not that it kept the "original"
transport from c; update the failure strings to say "does not equal supplied
transport (tr)" or "does not equal supplied transport (h.Transport)"
respectively in the initialized_client_with_transport and
initialized_client_with_http_client_and_transport subtests (and the analogous
check comparing to h.Transport), keeping references to cloned.client.Transport,
tr, h.Transport and WithTransport to locate the assertions.
In `@github/github.go`:
- Line 886: NewUploadRequest currently unconditionally sets
req.Header.Set("User-Agent", c.userAgent) which is inconsistent with NewRequest
and NewFormRequest (they only set the header when c.userAgent != ""), so update
NewUploadRequest to match by only setting the "User-Agent" header when
c.userAgent != "" (i.e., add the same guard used in NewRequest/NewFormRequest)
so a deliberate WithUserAgent("") results in no header across all request
constructors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a25d4c72-e72b-4c2a-800a-291f8aabf685
📒 Files selected for processing (42)
README.mdexample/actionpermissions/main.goexample/auditlogstream/main.goexample/basicauth/main.goexample/codespaces/newreposecretwithxcrypto/main.goexample/codespaces/newusersecretwithxcrypto/main.goexample/commitpr/main.goexample/contents/main.goexample/listenvironments/main.goexample/migrations/main.goexample/newfilewithappauth/main.goexample/newrepo/main.goexample/newreposecretwithxcrypto/main.goexample/otel/main.goexample/ratelimit/main.goexample/simple/main.goexample/tokenauth/main.goexample/topics/main.goexample/uploadreleaseassetfromrelease/main.goexample/verifyartifact/main.gogithub/actions_artifacts.gogithub/actions_artifacts_test.gogithub/actions_workflow_jobs.gogithub/actions_workflow_jobs_test.gogithub/actions_workflow_runs.gogithub/actions_workflow_runs_test.gogithub/actions_workflows_test.gogithub/copilot_test.gogithub/doc.gogithub/example_iterators_test.gogithub/examples_test.gogithub/github.gogithub/github_test.gogithub/rate_limit.gogithub/rate_limit_test.gogithub/repos_contents.gogithub/repos_contents_test.gogithub/repos_releases_test.gotest/fields/fields.gotest/integration/authorizations_test.gotest/integration/github_test.gotools/metadata/main.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Steve Hipwell steve.hipwell@gmail.com
Summary by CodeRabbit
NewClient()now returns(client, error)and requires explicit error handlingWithAuthToken(),WithEnterpriseURLs()) instead of method chaining