-
-
Notifications
You must be signed in to change notification settings - Fork 5
Fix regression in github enterprise url handling #40 #41
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
jferrl
left a comment
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.
Please, review comments. 😃
github_test.go
Outdated
| wantErr bool | ||
| name string | ||
| baseURL string | ||
| expectedBaseURL interface{} |
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.
Prefer keeping the original wantErr bool + a separate expectedURL string field instead of using interface{} to represent both. This is more idiomatic Go and gives type safety at compile time.
github_test.go
Outdated
| t.Errorf("withEnterpriseURL() error = %v, wantErr %v", err, tt.wantErr) | ||
| githubClient, err := client.withEnterpriseURL(tt.baseURL) | ||
|
|
||
| if err != nil { |
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 silently passes when an error occurs and expectedBaseURL is nil, but doesn't assert that an error was expected. The original if (err != nil) != tt.wantErr pattern is stricter — it catches both unexpected errors and missing expected errors.
jferrl
left a comment
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.
LGTM! Thanks!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #41 +/- ##
==========================================
+ Coverage 92.20% 92.50% +0.29%
==========================================
Files 3 3
Lines 154 160 +6
==========================================
+ Hits 142 148 +6
Misses 7 7
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Description
Resolves #40
Type of Change
Please delete options that are not relevant.
Testing
Documentation
Checklist
Screenshots (if applicable)
If your changes include visual elements, please add screenshots here.
Related Issues
Closes #(issue number)
Additional Notes
Add any other context about the pull request here.