Code Review with GitHub PRs
Pull requests are the integration gate for Go module repos.
Effective review keeps changes small, calls out go.mod impact, and verifies tests - not just style opinions.
Summary
A good Go PR states behavior change, module/API impact, and how tests were run.
Reviewers scan exported API deltas, error handling, concurrency, and dependency diffs before approving.
Large PRs stall trunk; slice by package or feature flag when possible.
Recipe
Quick-reference recipe card - copy-paste ready.
PR title: internal/parse: reject empty input in Decode
PR body template:
## Summary
- Reject empty input in `Decode` with `ErrEmptyInput`
## Module impact
- No new dependencies
- Exported error var is a compatible addition
## Test plan
- [ ] go test ./...
- [ ] go vet ./...
- [ ] go mod tidy (no diff)
## Screenshots / logs
N/AWhen to reach for this:
- Establishing team review norms for a new Go service or library.
- Training reviewers to spot breaking module changes.
- Reducing review latency on monorepos with noisy diffs.
- Onboarding contributors who only know language-level review.
Working Example
Contributor opens a PR adding gRPC health checks:
// internal/health/grpc.go (excerpt)
package health
import "google.golang.org/grpc/health/grpc_health_v1"
func Register(s *grpc.Server) {
srv := grpc_health_v1.NewHealthServer()
grpc_health_v1.RegisterHealthServer(s, srv)
}# go.mod excerpt
+ google.golang.org/grpc v1.68.0Reviewer checklist:
- Confirm
go test ./...green in CI. - Ask whether health should be optional via config.
- Verify gRPC version aligns with existing server setup.
- Check import path does not leak
internal/across module boundary. - Request changelog note if public operator-facing behavior changed.
Approval after author documents opt-out flag and tidy check passes.
What this demonstrates:
- API and dependency sections in description guide review focus.
- Reviewers treat
go.moddiff as first-class, not noise. - Behavior questions precede bike-shedding on naming.
Deep Dive
How It Works
- Author pushes branch and opens PR against
main. - CI runs tests, vet, tidy, lint.
- Reviewers leave comments on lines, packages, or module files.
- Author pushes fixes; GitHub re-runs checks.
- Maintainer merges when approvals and required checks satisfy branch protection.
GitHub's merge queue (optional) serializes merges to keep trunk green under load.
PR Size Guidance
| Size | Lines (approx) | Review outcome |
|---|---|---|
| Small | under 200 | Same-day merge likely |
| Medium | 200-400 | Needs focused review time |
| Large | 400+ | Split unless mechanical rename |
Mechanical changes (go fix, generated protobuf refresh) may exceed limits if isolated and labeled.
Module-Aware Review Points
| Area | Question |
|---|---|
| Exported symbols | Does this promise compatibility for module consumers? |
go.mod | New transitive weight? Version justified? |
internal/ | Any illegal cross-module internal import? |
| Errors | Wrapped with %w? Sentinel vs dynamic errors documented? |
| Concurrency | Goroutine lifecycle, context cancellation, data races? |
| Tests | Table-driven? Covers error paths? -race worth it? |
Description Fields That Help
- User-visible behavior - CLI flags, HTTP codes, config keys.
- Breaking changes - Requires
/v2path or major bump? - Rollout - Feature flag, migration steps.
- Benchmarks - If hot path changed, paste
benchstatsummary.
Reviewer Etiquette
Request changes for correctness, module safety, or missing tests.
Nit comments should be optional (nit: prefix).
Approve when issues are minor and trust author follow-up issues for debt.
Gotchas
- Rubber-stamping green CI - Tests may miss packages without cases. Fix: skim diff for untested exported funcs.
- Ignoring
go.sumbulk - Could hide supply-chain swap. Fix: readgo mod why -mfor new modules. - Style-only review on hot paths - Misses alloc regressions. Fix: ask for benchmark or profiling note.
- Giant generated diff - Hides manual edits. Fix: separate PRs for regen vs logic.
- Reviewing fork PRs without maintainer checkout - Hard to run locally. Fix: use
gh pr checkoutor trust CI matrix.
Alternatives
| Alternative | Use When | Don't Use When |
|---|---|---|
| Pair programming | Complex design spike | Async distributed team |
| Stacked PRs | Sequential dependent changes | Reviewers lack stack tooling |
| CODEOWNERS auto-request | Large monorepo ownership | Tiny two-person library |
| Maintainer merge without PR | Emergency hotfix | Normal feature flow |
FAQs
How small should a Go PR be?
Target one logical change reviewers can hold in working memory.
Split refactors from behavior changes across PRs.
Should reviewers run code locally?
For risky changes, yes: gh pr checkout <n> then go test ./....
CI green is necessary but not always sufficient.
How review dependency-only PRs?
Check changelog and security advisories for bumped modules.
Run tests and inspect go mod why for unexpected paths.
What labels help Go repos?
breaking, module-deps, needs-changelog, security.
Filter release notes and review priority.
Should API renames go in one PR?
Prefer mechanical rename PR separate from behavior changes.
Easier review and cleaner bisect.
How handle WIP PRs?
Use draft PRs.
Convert to ready only when description, tests, and tidy are complete.
Do comments on generated files matter?
Comment on generator inputs or Makefile targets, not *.pb.go lines directly.
Regenerate in follow-up commit.
How review context cancellation changes?
Trace call paths from HTTP/gRPC handlers to background work.
Missing ctx.Done() checks are release blockers.
Should libraries require changelog entries?
Yes for exported API or behavior consumers rely on.
Apps may use release notes only at tag time.
How fast should first review happen?
Within one business day keeps trunk flow healthy.
Set team SLA and rotate review duty.
What about auto-merge?
Fine when required checks are strong and PR size limits enforced.
Avoid for dependency bumps without human glance.
How review monorepo cross-module PRs?
List every touched go.mod.
Confirm workspace or replace directives are not accidentally committed.
Related
- Branching Strategies & Trunk-Based Flow - Short branches make review tractable
- GitHub Actions for Go Repos - CI reviewers rely on
- Git for Go Teams - Why merged PRs become module releases
- Signed Tags & Go Module Releases - Post-merge tagging discipline
- Git & GitHub Best Practices - Portable review and Git habits
Stack versions: This page was written for Go 1.26.x (Green Tea GC default, go fix modernizers - verify patch at build), chi (latest - verify at build), gin (latest - verify at build), echo (latest - verify at build), google.golang.org/grpc (latest - verify at build), sigs.k8s.io/controller-runtime (latest - verify at build), kubebuilder (latest - verify at build), tinygo (latest - verify board targets at build), wazero (latest - verify at build), and golangci-lint (latest - verify linter set at build).