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

feat(vcs): extract interfaces and types from gitea/ (Phase 1) #83

Merged
aweiker merged 4 commits from review-bot-issue-78 into feature/github-support 2026-05-12 19:14:45 +00:00
Conversation 13 Commits 4 Files Changed 3
+16 -7
2 changed files with 16 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit c889724dda - Show all commits
vcs/check.go
+1 -1
View File
@@ -1,7 +1,7 @@
//go:build ignore
gpt-review-bot commented 2026-05-12 19:02:18 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The compile-time assertion file imports the gitea adapter directly from within package vcs. While gated by //go:build ignore, consider placing this assertion in a separate build-tagged test or a _test.go file to further reduce the chance of accidental inclusion creating an unwanted dependency.

**[NIT]** The compile-time assertion file imports the gitea adapter directly from within package vcs. While gated by //go:build ignore, consider placing this assertion in a separate build-tagged test or a _test.go file to further reduce the chance of accidental inclusion creating an unwanted dependency.
// This file is excluded from normal builds.
sonnet-review-bot commented 2026-05-12 17:06:39 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The instruction 'Run go build -v . inside this file to see the documented gaps' is slightly misleading — you can't run a build inside a file. The intended instruction is probably 'Remove the //go:build ignore tag and run go build ./vcs/' or 'Run go build -tags ignore ./vcs/'. A clearer instruction avoids future confusion.

**[NIT]** The instruction 'Run `go build -v .` inside this file to see the documented gaps' is slightly misleading — you can't run a build inside a file. The intended instruction is probably 'Remove the //go:build ignore tag and run `go build ./vcs/`' or 'Run `go build -tags ignore ./vcs/`'. A clearer instruction avoids future confusion.
// Run `go build -v .` inside this file to see the documented gaps between
// Remove the //go:build ignore tag and run `go build ./vcs/` to see the documented gaps between
gpt-review-bot commented 2026-05-12 17:07:56 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The instruction "Run go build -v . inside this file" is misleading. With //go:build ignore, this file is excluded by default. Suggest clarifying to "Run go build -tags ignore ./vcs" (or ./...) to trigger the intentional compile-time assertion.

**[NIT]** The instruction "Run `go build -v .` inside this file" is misleading. With //go:build ignore, this file is excluded by default. Suggest clarifying to "Run `go build -tags ignore ./vcs`" (or `./...`) to trigger the intentional compile-time assertion.
// gitea.Client and vcs.Client that Phase 2 (Gitea adapter) must bridge.
//
// Known gaps (as of Phase 1):
1
vcs/types.go
+15 -6
View File
@@ -1,14 +1,23 @@
// Package vcs defines the shared interface and types for VCS platform clients.
// Adapters (gitea, github) implement these interfaces; the core review logic
// uses them without knowing the underlying platform.
package vcs
sonnet-review-bot commented 2026-05-12 19:07:24 +00:00
Review
Copy Link
Copy Source

[NIT] The types.go file is missing a package doc comment or any reference back to the main package-level documentation. Since interfaces.go carries the package doc (// Package vcs defines...), this is fine per Go convention (only one file needs the package comment), but it's worth noting the package doc lives in interfaces.go rather than a dedicated doc.go, which is a slightly non-idiomatic placement for packages that will grow to have many files.

**[NIT]** The `types.go` file is missing a package doc comment or any reference back to the main package-level documentation. Since `interfaces.go` carries the package doc (`// Package vcs defines...`), this is fine per Go convention (only one file needs the package comment), but it's worth noting the package doc lives in `interfaces.go` rather than a dedicated `doc.go`, which is a slightly non-idiomatic placement for packages that will grow to have many files.
// ReviewEvent is the event type for a pull request review action.
type ReviewEvent string
const (
// ReviewEventApprove approves the pull request.
sonnet-review-bot commented 2026-05-12 17:06:39 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The package doc comment in types.go duplicates the one in interfaces.go almost verbatim. Only one file in a package should carry the package doc comment; the others should have no package comment or just package vcs with no preceding comment. The authoritative doc comment is on interfaces.go — remove the comment block from types.go.

**[MINOR]** The package doc comment in types.go duplicates the one in interfaces.go almost verbatim. Only one file in a package should carry the package doc comment; the others should have no package comment or just `package vcs` with no preceding comment. The authoritative doc comment is on interfaces.go — remove the comment block from types.go.
ReviewEventApprove ReviewEvent = "APPROVED"
gpt-review-bot commented 2026-05-12 19:02:18 +00:00
Review
Copy Link
Copy Source

[MINOR] ReviewEventApprove is set to "APPROVED", which matches a review state rather than an action. If these constants represent actions to post (as implied by ReviewRequest.Event), consider using "APPROVE" to align with GitHub semantics, or clearly document that adapters must translate these abstracted values to provider-specific event strings.

**[MINOR]** ReviewEventApprove is set to "APPROVED", which matches a review state rather than an action. If these constants represent actions to post (as implied by ReviewRequest.Event), consider using "APPROVE" to align with GitHub semantics, or clearly document that adapters must translate these abstracted values to provider-specific event strings.
// ReviewEventRequestChanges requests changes to the pull request.
ReviewEventRequestChanges ReviewEvent = "REQUEST_CHANGES"
gpt-review-bot commented 2026-05-12 17:07:56 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Acronym casing: field name "Sha" should be "SHA" to follow Go acronym capitalization conventions (see style pattern: Acronyms Are All-Caps). The JSON tag can remain "sha".

**[MINOR]** Acronym casing: field name "Sha" should be "SHA" to follow Go acronym capitalization conventions (see style pattern: Acronyms Are All-Caps). The JSON tag can remain "sha".
// ReviewEventComment posts a review comment without approval or rejection.
ReviewEventComment ReviewEvent = "COMMENT"
)
// PullRequest holds relevant PR metadata.
type PullRequest struct {
Title string `json:"title"`
sonnet-review-bot commented 2026-05-12 19:01:15 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The anonymous struct for the Head field works but embedding a named struct would be slightly cleaner and more testable (allows construction via named literal without field-name lookup). This is a minor style preference; anonymous embedded structs in this pattern are common in JSON-mapping code.

**[NIT]** The anonymous struct for the Head field works but embedding a named struct would be slightly cleaner and more testable (allows construction via named literal without field-name lookup). This is a minor style preference; anonymous embedded structs in this pattern are common in JSON-mapping code.
Body string `json:"body"`
Head struct {
Sha string `json:"sha"`
SHA string `json:"sha"`
Ref string `json:"ref"`
} `json:"head"`
}
4
 @@ -56,7 +65,7 @@ type ReviewComment struct {
type ReviewRequest struct {
// Body is the top-level review comment.
Body string `json:"body"`
// Event is "APPROVED" or "REQUEST_CHANGES".
Event string `json:"event"`
// Event is the review action (approve, request changes, or comment).
Event ReviewEvent `json:"event"`
Comments []ReviewComment `json:"comments,omitempty"`
}