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

feat(github): implement Reviewer and Identity interfaces (#81) #105

Merged
aweiker merged 7 commits from review-bot-issue-81 into feature/github-support 2026-05-13 13:39:14 +00:00
Conversation 40 Commits 7 Files Changed 7
+12 -6
2 changed files with 12 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 8b256360bf - Show all commits
github/review.go
+8 -2
View File
3
 @@ -66,9 +66,15 @@ func translateGitHubReviewState(state string) string {
}
// PostReview submits a review on a pull request.
// The ReviewRequest.Event values (APPROVE, REQUEST_CHANGES, COMMENT) are sent
// directly — they match GitHub's canonical event strings.
//
// The vcs.ReviewEvent constants (ReviewEventApprove, ReviewEventRequestChanges,
// ReviewEventComment) have string values that match GitHub's wire-format event
// strings (APPROVE, REQUEST_CHANGES, COMMENT), so Event is cast directly to
// string without translation.
//
// ReviewComment.Position maps directly to the GitHub API position field.
// When req.Comments is empty, the payload omits the comments field entirely
// (via the omitempty tag on postReviewRequest.Comments).
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
sonnet-review-bot commented 2026-05-13 07:55:21 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] PostReview doc comment says 'ReviewRequest.Event values (APPROVE, REQUEST_CHANGES, COMMENT) are sent directly — they match GitHub's canonical event strings.' This is misleading: GitHub's canonical event strings are actually APPROVE, REQUEST_CHANGES, and COMMENT, but the vcs type uses ReviewEventApprove etc. which the test confirms maps to 'APPROVE'. The comment is correct about the wire format but conflates the vcs abstraction with the GitHub API. More importantly, when a review has no comments, payload.Comments will be nil (not assigned), which is fine since it's omitempty, but this is an implicit assumption worth noting.

**[MINOR]** PostReview doc comment says 'ReviewRequest.Event values (APPROVE, REQUEST_CHANGES, COMMENT) are sent directly — they match GitHub's canonical event strings.' This is misleading: GitHub's canonical event strings are actually APPROVE, REQUEST_CHANGES, and COMMENT, but the vcs type uses ReviewEventApprove etc. which the test confirms maps to 'APPROVE'. The comment is correct about the wire format but conflates the vcs abstraction with the GitHub API. More importantly, when a review has no comments, `payload.Comments` will be nil (not assigned), which is fine since it's `omitempty`, but this is an implicit assumption worth noting.
13
github/review_test.go
+4 -4
View File
9
 @@ -362,8 +362,8 @@ func TestDismissReview_401(t *testing.T) {
func TestTranslateGitHubReviewState(t *testing.T) {
tests := []struct {
input string
expected string
input string
want string
}{
{"APPROVED", "APPROVED"},
{"CHANGES_REQUESTED", "REQUEST_CHANGES"},
@@ -374,8 +374,8 @@ func TestTranslateGitHubReviewState(t *testing.T) {
}
for _, tt := range tests {
got := translateGitHubReviewState(tt.input)
if got != tt.expected {
t.Errorf("translateGitHubReviewState(%q) = %q, want %q", tt.input, got, tt.expected)
if got != tt.want {
t.Errorf("translateGitHubReviewState(%q) = %q, want %q", tt.input, got, tt.want)
}
}
sonnet-review-bot commented 2026-05-13 07:26:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] containsStr and containsSubstring reimplement strings.Contains from the standard library. The repository conventions do not restrict use of the standard library; strings.Contains(s, substr) should be used directly. The current implementation is also slightly incorrect: containsStr returns true when s == substr even without going through containsSubstring, but the logic mixing len checks and || chains is harder to read and maintain than a direct strings.Contains call.

**[MINOR]** containsStr and containsSubstring reimplement strings.Contains from the standard library. The repository conventions do not restrict use of the standard library; `strings.Contains(s, substr)` should be used directly. The current implementation is also slightly incorrect: `containsStr` returns true when `s == substr` even without going through `containsSubstring`, but the logic mixing `len` checks and `||` chains is harder to read and maintain than a direct `strings.Contains` call.
}
sonnet-review-bot commented 2026-05-13 07:37:01 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] There are two trailing blank lines at the end of review_test.go. gofmt typically produces a single trailing newline; this is minor but inconsistent with the style pattern.

**[NIT]** There are two trailing blank lines at the end of review_test.go. gofmt typically produces a single trailing newline; this is minor but inconsistent with the style pattern.