Fix regression in github enterprise url handling #40#41
Fix regression in github enterprise url handling #40#41luna-veil-8080 wants to merge 1 commit intojferrl:mainfrom
Conversation
| wantErr bool | ||
| name string | ||
| baseURL string | ||
| expectedBaseURL interface{} |
There was a problem hiding this comment.
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.
| @@ -12,33 +12,48 @@ import ( | |||
|
|
|||
| func Test_githubClient_withEnterpriseURL(t *testing.T) { | |||
There was a problem hiding this comment.
Could you add test cases for the remaining branches?
https://github.example.com/api/v3/ — already fully suffixed, should stay as-is
https://ghes.api.example.com — .api. in host, should skip /api/v3/ append
https://github.example.com/ — trailing slash already present, should not double it
| 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.
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.
2 participants
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.