feat: Github code search api integration using GO GRPC #1
Conversation
README.md
Outdated
| 1. Ensure you have the following installed on your system: | ||
| Go (1.23.3) - Download & Install (https://go.dev/dl/) | ||
|
|
||
| 2. Protocol Buffers Compiler (protoc) - `brew install protobuf` | ||
|
|
||
| 3. Buf (for Protobuf management) - Install using Homebrew: `brew install bufbuild/buf/buf` | ||
|
|
||
| 4. Git - Install using Homebrew: `brew install git` | ||
|
|
||
| 5. Make - Install using Homebrew: `brew install make` |
There was a problem hiding this comment.
This assumes the user is on OSX. You should include instructions for Linux users.
There was a problem hiding this comment.
Also, why are there extra newlines between list items for only this list?
There was a problem hiding this comment.
I'll use docker to avoid the local setup challenges and make local setup consistent regardless of the OS.
Also, I'll remove the newlines if not required.
README.md
Outdated
| 3. To execute a test request, run: `make test` | ||
|
|
||
| ## Troubleshooting | ||
| 1. Missing Dependencies: Run go mod tidy to install missing Go dependencies. |
There was a problem hiding this comment.
| 1. Missing Dependencies: Run go mod tidy to install missing Go dependencies. | |
| 1. Missing Dependencies: Run `go mod tidy` to install missing Go dependencies. |
README.md
Outdated
|
|
||
| ## Troubleshooting | ||
| 1. Missing Dependencies: Run go mod tidy to install missing Go dependencies. | ||
| 2. Protobuf Compilation Issues: Ensure protoc and buf are correctly installed. |
There was a problem hiding this comment.
| 2. Protobuf Compilation Issues: Ensure protoc and buf are correctly installed. | |
| 2. Protobuf Compilation Issues: Ensure `protoc` and `buf` are correctly installed. |
README.md
Outdated
| ## Troubleshooting | ||
| 1. Missing Dependencies: Run go mod tidy to install missing Go dependencies. | ||
| 2. Protobuf Compilation Issues: Ensure protoc and buf are correctly installed. | ||
| 3. Authentication Errors: Ensure you have set GITHUB_API_TOKEN with a valid GitHub token in your env variable. |
There was a problem hiding this comment.
| 3. Authentication Errors: Ensure you have set GITHUB_API_TOKEN with a valid GitHub token in your env variable. | |
| 3. Authentication Errors: Ensure you have set `GITHUB_API_TOKEN` with a valid GitHub token in your env variable. |
makefile
Outdated
| go mod vendor | ||
|
|
||
| test: | ||
| go test ./... |
server/github/client_test.go
Outdated
| "items": [ | ||
| { | ||
| "html_url": "https://github.com/test/repo/blob/main/file.go", | ||
| "repository": { | ||
| "full_name": "test/repo" | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
This seems to be the same content as in the mock client. You might consider pulling it out into a package const.
server/github/client_test.go
Outdated
| // Set up test environment | ||
| os.Setenv("GITHUB_TOKEN", "test-token") | ||
| client := NewGitHubClient(mockServer.URL) |
There was a problem hiding this comment.
Using an env variable in tests make them brittle and hard/impossible to run concurrently. You may wan to refactor your client to take the token as a parameter instead.
There was a problem hiding this comment.
used option pattern to pass token and fetched the default token from environment
server/github/client_test.go
Outdated
| assert.NotNil(t, data) | ||
| assert.Len(t, data.Items, 1) | ||
| assert.Equal(t, "https://github.com/test/repo/blob/main/file.go", data.Items[0].HTMLURL) | ||
| assert.Equal(t, "test/repo", data.Items[0].Repository.FullName) |
There was a problem hiding this comment.
assert.DeepEqual is likely a better choice here. It is more readable and comprehensive.
There was a problem hiding this comment.
assert doesn't have DeepEqual, i've used assert.EqualValues instead
server/http_client/client.go
Outdated
| return fmt.Errorf("API error: %s", resp.Status) | ||
| } | ||
|
|
||
| return json.NewDecoder(resp.Body).Decode(response) |
There was a problem hiding this comment.
This is a really common bug and miss-use with the json package: golang/go#36225
You should rewrite this to not be susceptible to that bug.
There was a problem hiding this comment.
I've used json.Unmarshal
server/http_client/client.go
Outdated
| func setHeaders(req *http.Request, headers map[string]string) { | ||
| for key, value := range headers { | ||
| req.Header.Set(key, value) | ||
| } | ||
| } |
There was a problem hiding this comment.
This function only appears to be used once. You are probably better off just doing this in the once place it is needed instead of factoring it out to a function.
What?
Local Test Result: