-
Notifications
You must be signed in to change notification settings - Fork 152
fix: replace log.Fatal with proper error returns in client constructors #327
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Replace all log.Fatal calls in client constructor functions with proper error returns to allow library users to handle errors gracefully instead of crashing the entire application. Changes: - Updated NewOAuthClientCredentials to return (*Client, error) - Updated NewOAuth to return (*Client, error) - Updated NewOAuthWithCode to return (*Client, string, error) - Updated NewOAuthWithRefreshToken to return (*Client, string, error) - Updated NewOAuthbearerToken to return (*Client, error) - Updated NewBasicAuth to return (*Client, error) - Updated injectClient to return (*Client, error) - Removed unused log import - Updated all test files to handle the new error returns This ensures the library behaves as a proper library and doesn't force the calling application to exit on authentication errors. Fixes #326 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR removes log.Fatal calls from client constructor functions and replaces them with proper error returns, allowing library users to handle authentication errors gracefully instead of experiencing forced application exits.
Key Changes:
- Modified all client constructor functions to return errors alongside client instances
- Removed the
logpackage import fromclient.go - Updated all test files to handle the new error return values
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| client.go | Modified 7 constructor functions to return (*Client, error) and replaced log.Fatal with fmt.Errorf; removed unused log import |
| tests/workspace_test.go | Updated getBitbucketClient helper to handle error return from NewBasicAuth |
| tests/webhooks_test.go | Added error handling for NewBasicAuth call |
| tests/variable_test.go | Added error handling for NewBasicAuth call and reordered imports |
| tests/user_test.go | Added error handling for NewBasicAuth call |
| tests/ssh_test.go | Added error handling for NewBasicAuth call |
| tests/repository_test.go | Added error handling for all NewBasicAuth calls across multiple test functions |
| tests/repositories_test.go | Added error handling for NewBasicAuth calls |
| tests/project_test.go | Updated getClient helper to handle error return from NewBasicAuth |
| tests/list_test.go | Added error handling for NewBasicAuth call |
| tests/environment_test.go | Added error handling for NewBasicAuth calls |
| tests/diff_test.go | Added error handling for NewBasicAuth calls |
| tests/deploykeys_test.go | Added error handling for NewBasicAuth calls |
| tests/client_test.go | Added error handling for NewBasicAuth call |
| tests/branchrestrictions_test.go | Updated setup helper to handle error return from NewBasicAuth |
| teams.go | Removed trailing newline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary of ChangesHello @ktrysmt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness and usability of the library by transitioning from abrupt application termination via Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This is an excellent pull request that significantly improves the library's robustness by replacing log.Fatal calls with proper error returns in client constructors. This change allows consumers of the library to handle authentication and setup errors gracefully. The modifications are thorough, and all tests have been updated accordingly, which is great to see. I have a few minor suggestions to further enhance error handling by adding more context to returned errors and a design recommendation for the NewOAuth function to improve its usability as a library component.
| } | ||
|
|
||
| func NewOAuth(i, s string) *Client { | ||
| func NewOAuth(i, s string) (*Client, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function NewOAuth interacts directly with stdin and stdout (fmt.Printf and fmt.Scan). This is generally considered an anti-pattern for a library function as it makes it difficult to use in non-interactive environments (e.g., web servers, background jobs) and also makes the function hard to test.
I recommend refactoring this to improve modularity and usability. You could split the logic:
- Provide a function that generates and returns the authentication URL.
- The library user can then display this URL, obtain the authorization code through their own UI/CLI, and use the existing
NewOAuthWithCodefunction.
This approach would make the library more flexible. The current NewOAuth implementation could be moved to an example file or a separate CLI tool package to demonstrate interactive usage.
| if err != nil { | ||
| return nil, "", err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve error diagnostics, it's a good practice to wrap the error returned from injectClient to provide more context. This creates a chain of errors that is easier for the caller to inspect and understand.
| if err != nil { | |
| return nil, "", err | |
| } | |
| if err != nil { | |
| return nil, "", fmt.Errorf("failed to create client: %w", err) | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback! I've updated the code to wrap the error from injectClient with additional context. This has been addressed in commit bf2edad.
| if err != nil { | ||
| return nil, "", err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the comment in NewOAuthWithCode, it would be beneficial to wrap the error returned from injectClient here as well. This will provide more context to the caller, making debugging easier.
| if err != nil { | |
| return nil, "", err | |
| } | |
| if err != nil { | |
| return nil, "", fmt.Errorf("failed to create client: %w", err) | |
| } |
Improve error diagnostics by wrapping errors returned from injectClient with additional context in NewOAuthWithCode and NewOAuthWithRefreshToken. This creates a clearer error chain for callers to inspect and debug. Changes: - Wrap injectClient error in NewOAuthWithCode with "failed to create client" - Wrap injectClient error in NewOAuthWithRefreshToken with "failed to create client" This addresses review feedback from @gemini-code-assist.
The NewOAuth function uses stdin/stdout directly, making it unsuitable for non-interactive environments such as web servers and background jobs. This marks the function as deprecated and recommends using NewOAuthWithCode instead, where users can obtain the authorization code through their own UI/CLI implementation. This addresses the design concern raised in review feedback from @gemini-code-assist.
Summary
This PR addresses issue #326 by replacing all
log.Fatalcalls in client constructor functions with proper error returns. This allows library users to handle errors gracefully instead of having the entire application crash on authentication errors.Changes
Modified Functions
NewOAuthClientCredentials: Now returns(*Client, error)instead of*ClientNewOAuth: Now returns(*Client, error)instead of*ClientNewOAuthWithCode: Now returns(*Client, string, error)instead of(*Client, string)NewOAuthWithRefreshToken: Now returns(*Client, string, error)instead of(*Client, string)NewOAuthbearerToken: Now returns(*Client, error)instead of*ClientNewBasicAuth: Now returns(*Client, error)instead of*ClientinjectClient(internal): Now returns(*Client, error)instead of*ClientAdditional Changes
logimport fromclient.gofmt.Errorfwith%wfor error wrappingImpact
This is a breaking change for existing users of this library. Any code that calls these constructor functions will need to be updated to handle the returned error.
Migration Example
Before:
After:
Testing
go fmtBenefits
Fixes #326