rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

feat(vcs): complete Phase 1 — util.go, type cleanup, interface additions (fixes #84, #85, #86) #88

Merged
aweiker merged 3 commits from review-bot-issue-84 into feature/github-support 2026-05-12 20:18:18 +00:00
Conversation 17 Commits 3 Files Changed 9
+35 -10
5 changed files with 35 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ec03dc2373 - Show all commits
cmd/review-bot/main.go
+3 -3
View File
@@ -838,9 +838,9 @@ func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path
return result, nil
sonnet-review-bot commented 2026-05-12 19:57:18 +00:00
Review
Copy Link
Copy Source

[NIT] The giteaClientAdapter.GetFileContent method has a parameter named filepath which shadows the standard library package path/filepath. Although path/filepath is not imported in this file, the name filepath for a string parameter is mildly confusing. Renaming to filePath or path would be cleaner and is consistent with how the vcs.FileReader interface parameter is described elsewhere.

**[NIT]** The giteaClientAdapter.GetFileContent method has a parameter named `filepath` which shadows the standard library package `path/filepath`. Although `path/filepath` is not imported in this file, the name `filepath` for a string parameter is mildly confusing. Renaming to `filePath` or `path` would be cleaner and is consistent with how the vcs.FileReader interface parameter is described elsewhere.
}
func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filePath, ref string) (string, error) {
if ref != "" {
return a.client.GetFileContentRef(ctx, owner, repo, filepath, ref)
return a.client.GetFileContentRef(ctx, owner, repo, filePath, ref)
}
return a.client.GetFileContent(ctx, owner, repo, filepath)
return a.client.GetFileContent(ctx, owner, repo, filePath)
}
gitea/client.go
+1 -1
View File
2
 @@ -835,7 +835,7 @@ func (c *Client) ResolveComment(ctx context.Context, owner, repo string, comment
// DismissReview dismisses a review on a pull request.
// This is a stub for the vcs.Reviewer interface; full implementation is Phase 2.
func (c *Client) DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error {
return fmt.Errorf("DismissReview: %w", errors.ErrUnsupported)
return fmt.Errorf("dismiss review %d on %s/%s#%d: %w", reviewID, owner, repo, number, errors.ErrUnsupported)
}
sonnet-review-bot commented 2026-05-12 20:08:36 +00:00
Review
Copy Link
Copy Source

[NIT] GetFileContentAtRef is a thin wrapper that does nothing but delegate to GetFileContentRef with the same arguments in the same order. The docstring says "This delegates to GetFileContentRef for the Gitea implementation" — since the method exists only to satisfy the vcs.PRReader interface, a one-liner return c.GetFileContentRef(ctx, owner, repo, path, ref) with no further ceremony would be ideal, which is already the case. No change needed, but consider whether GetFileContentRef should simply be renamed to GetFileContentAtRef to avoid the two names existing in parallel on the same type.

**[NIT]** `GetFileContentAtRef` is a thin wrapper that does nothing but delegate to `GetFileContentRef` with the same arguments in the same order. The docstring says "This delegates to GetFileContentRef for the Gitea implementation" — since the method exists only to satisfy the `vcs.PRReader` interface, a one-liner `return c.GetFileContentRef(ctx, owner, repo, path, ref)` with no further ceremony would be ideal, which is already the case. No change needed, but consider whether `GetFileContentRef` should simply be renamed to `GetFileContentAtRef` to avoid the two names existing in parallel on the same type.
// GetFileContentAtRef fetches a file at a specific ref from a repo.
gitea/conformance_test.go
+25
View File
@@ -0,0 +1,25 @@
//go:build phase2
package gitea_test
import (
"gitea.weiker.me/rodin/review-bot/gitea"
"gitea.weiker.me/rodin/review-bot/vcs"
)
// Compile-time interface conformance assertions.
// These will verify gitea.Client satisfies vcs interfaces once the Phase 2
// adapter bridges the method signature gaps:
//
// - PRReader: GetPullRequest returns *gitea.PullRequest (needs *vcs.PullRequest)
// - PRReader: GetPullRequestFiles returns []gitea.ChangedFile (needs []vcs.ChangedFile)
// - FileReader: GetFileContent lacks ref parameter
// - Reviewer: PostReview uses (event, body, comments) instead of vcs.ReviewRequest
//
// Remove the phase2 build tag once the adapter is complete.
var (
_ vcs.PRReader = (*gitea.Client)(nil)
sonnet-review-bot commented 2026-05-12 20:08:36 +00:00
Review
Copy Link
Copy Source

[NIT] The conformance assertions under //go:build phase2 include vcs.PRReader and vcs.Reviewer which gitea.Client explicitly does NOT yet satisfy (wrong return types for GetPullRequest, GetPullRequestFiles, PostReview). The build tag correctly guards against this, but the comment block lists the gaps well. This is fine as-is — just noting that once Phase 2 lands this file should have the build tag removed rather than being left indefinitely.

**[NIT]** The conformance assertions under `//go:build phase2` include `vcs.PRReader` and `vcs.Reviewer` which `gitea.Client` explicitly does NOT yet satisfy (wrong return types for `GetPullRequest`, `GetPullRequestFiles`, `PostReview`). The build tag correctly guards against this, but the comment block lists the gaps well. This is fine as-is — just noting that once Phase 2 lands this file should have the build tag removed rather than being left indefinitely.
_ vcs.FileReader = (*gitea.Client)(nil)
_ vcs.Reviewer = (*gitea.Client)(nil)
_ vcs.Identity = (*gitea.Client)(nil)
)
vcs/util.go
+2 -2
View File
1
 @@ -37,7 +37,7 @@ func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path
var walk func(string) error
walk = func(dir string) error {
if err := ctx.Err(); err != nil {
return fmt.Errorf("context cancelled during traversal: %w", err)
return fmt.Errorf("context canceled during traversal: %w", err)
}
entries, err := client.ListContents(ctx, owner, repo, dir)
sonnet-review-bot commented 2026-05-12 20:08:36 +00:00
Review
Copy Link
Copy Source

[MINOR] There is a functional duplication between vcs.GetAllFilesInPath and gitea.Client.GetAllFilesInPath. The latter already has identical logic (though with warn-and-continue error handling instead of fail-fast). The doc comment on the vcs version explicitly calls out the difference, which is good, but over time both implementations will need to be kept in sync. Consider whether gitea.Client.GetAllFilesInPath should be refactored to delegate to vcs.GetAllFilesInPath via the vcs.FileReader interface (which gitea.Client will satisfy in Phase 2) to eliminate the duplication entirely.

**[MINOR]** There is a functional duplication between `vcs.GetAllFilesInPath` and `gitea.Client.GetAllFilesInPath`. The latter already has identical logic (though with warn-and-continue error handling instead of fail-fast). The doc comment on the vcs version explicitly calls out the difference, which is good, but over time both implementations will need to be kept in sync. Consider whether `gitea.Client.GetAllFilesInPath` should be refactored to delegate to `vcs.GetAllFilesInPath` via the `vcs.FileReader` interface (which `gitea.Client` will satisfy in Phase 2) to eliminate the duplication entirely.
2
 @@ -47,7 +47,7 @@ func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path
for _, entry := range entries {
if err := ctx.Err(); err != nil {
return fmt.Errorf("context cancelled during traversal: %w", err)
return fmt.Errorf("context canceled during traversal: %w", err)
}
switch entry.Type {
4
vcs/util_test.go
+4 -4
View File
1
 @@ -2,8 +2,8 @@ package vcs_test
import (
"context"
"strings"
"fmt"
"strings"
"testing"
"gitea.weiker.me/rodin/review-bot/vcs"
@@ -306,7 +306,7 @@ func TestGetAllFilesInPath_ErrorPropagation(t *testing.T) {
}
})
t.Run("cancelled context propagates", func(t *testing.T) {
t.Run("canceled context propagates", func(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
cancel() // Cancel immediately
@@ -322,9 +322,9 @@ func TestGetAllFilesInPath_ErrorPropagation(t *testing.T) {
}
_, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
if err == nil {
t.Fatal("expected error from cancelled context, got nil")
t.Fatal("expected error from canceled context, got nil")
}
if !strings.Contains(err.Error(), "context cancelled") {
if !strings.Contains(err.Error(), "context canceled") {
t.Errorf("expected context cancellation error, got: %v", err)
}
})