Fundamental #

The Pull Request (PR) is one of the activities engineers do most often yet understand least deeply. For some teams, a PR is a formality — a checklist before merging. For more mature teams, the PR is the center of engineering collaboration: where technical decisions are discussed, knowledge spreads, and code quality is guarded before touching production. The difference between these two teams isn’t the tool they use — it’s how they make sense of PRs and code reviews. This article covers the most fundamental foundations of this practice.

What Is a Pull Request? #

A Pull Request is a mechanism for proposing code changes from one branch to another — usually from a feature branch to main or develop — so the changes can be reviewed, discussed, and validated before becoming part of the main codebase.

But that technical definition is only half the story. More deeply, a PR is a social contract between engineers:

A PR as a social contract means:
  ✓ The author commits: "This code I've thought through, tested, and I'm ready
                        to be accountable for. I'm inviting other perspectives."
  ✓ The reviewer commits: "I'll give constructive, timely feedback
                           to help this code become better."
  ✓ The team commits: "No code enters the main codebase without going through this process."

A PR isn’t “merge permission from a superior” and isn’t “a procedural step to get through”. It’s a collaboration mechanism designed to make better technical decisions than any single engineer could produce alone.

What Is a Code Review? #

A Code Review is the process of reviewing code changes in a PR — reading them, understanding their context, questioning their assumptions, and giving feedback that helps the author produce better code.

Key word: helping, not judging. A good code review is a technical dialogue between two engineers who both want the codebase to improve. Not an audit, not an interrogation, not a session proving who’s smarter.

flowchart LR
    subgraph WrongPerception
        A1["Author\nwrites code"] --> B1["Reviewer\nlooks for mistakes"]
        B1 --> C1["Author\ndefends themselves"]
        C1 --> D1["Tension,\nslow reviews"]
    end

    subgraph RightPerception
        A2["Author\nwrites code"] --> B2["Reviewer\nhelps improve"]
        B2 --> C2["Author\naccepts & discusses"]
        C2 --> D2["Better code,\nboth learn"]
    end

Why Are PRs and Code Reviews Important? #

A Quality Gate Before Production #

Bugs found at code review are far cheaper to fix than bugs found in production. No monitoring or alerting can replace a pair of engineer eyes that understand both business and technical context.

The cost of finding bugs at different stages:
  At code review    → edit code, push again (minutes to hours)
  At QA/staging     → assign to a developer, fix, retest (days)
  In production     → hotfix, incident response, potential data impact (days to weeks)

// One code review comment preventing a production bug
// can save 10x the team's time

Distributing Knowledge #

Code never reviewed is code understood by only one person. Every seriously reviewed PR is a knowledge transfer session — the reviewer learns about the change, the author learns from a different perspective.

flowchart TD
    PR[Pull Request] --> A["Author\ngains new perspectives"]
    PR --> B["Reviewer\nunderstands system changes"]
    PR --> C["Team reading the thread\nlearns from the discussion"]
    PR --> D["Future engineers\ncan read context in PR comments"]

Building Code Standards Organically #

Teams that consistently do code reviews automatically build shared understanding about what counts as good code — without needing long standard documents. These standards form from discussions happening in PRs, not from rules imposed from above.

Being Technical Decision Documentation #

PR discussions are the most honest documentation of why code was written a certain way. When there’s a question of “why did we choose this approach back then?”, the answer is often in an old PR thread.

Foundational Philosophy to Uphold #

Code Is a Team Asset, Not an Individual’s Property #

This is the most fundamental mindset shift. When engineers treat code as “my code”, they tend to get defensive when reviewed. When code is treated as a team asset, review becomes collaboration to improve a shared asset.

// ✗ Individual mindset

"This is my code, you don't need to change the way I code"

"I already have my own way, no need for review"

// ✓ Team mindset

"This code will be maintained by the team for years — I want
 to make sure others can understand and change it easily"

"This review helps the code entering the team codebase become better"

Review Code, Not People #

All code review feedback must be directed at the code and technical decisions — not at the author’s character, ability, or intelligence.

// ✗ Reviews attacking the person

"This is wrong. Why didn't you think first?"

"This approach shows you don't understand our architecture"

// ✓ Reviews focused on code

"This approach could cause a race condition with concurrent requests.
 Consider using a mutex here — here's a relevant example: [link]"

"There's a more idiomatic way to do this in Go: [example code]
 This will be easier for newly joined engineers to read"

Clarity Over Cleverness #

Clever, hard-to-understand code is a burden, not an asset. Clear, easily understood code — even if slightly more verbose — is far more valuable in the long run.

// ✗ Clever but hard to understand
result := lo.Filter(users, func(u User, _ int) bool { return u.Active && u.Age >= 18 && !u.Banned })

// ✓ Clear and easy to understand
eligibleUsers := filterEligibleUsers(users)

func filterEligibleUsers(users []User) []User {
    var eligible []User
    for _, u := range users {
        if u.Active && u.Age >= 18 && !u.Banned {
            eligible = append(eligible, u)
        }
    }
    return eligible
}

A PR Is a Conversation, Not a Checklist #

A good PR isn’t done just because every comment has been responded to. It’s done when all parties feel the code about to be merged is the best version producible with the time and context available.

The Pull Request Lifecycle #

Understanding the PR lifecycle helps engineers know their roles and responsibilities at each stage:

stateDiagram-v2
    [*] --> Draft: Author starts coding
    Draft --> ReadyForReview: Self-review done, CI passes
    ReadyForReview --> InReview: Reviewer starts reading
    InReview --> ChangesRequested: Reviewer finds things to fix
    ChangesRequested --> ReadyForReview: Author completes feedback
    InReview --> Approved: Reviewer satisfied with changes
    Approved --> Merged: Merge to target branch
    Merged --> [*]
    InReview --> Closed: PR not continued
    Closed --> [*]

Fundamental Pull Request Principles (Author Side) #

One PR, One Purpose #

A PR mixing a new feature, a big refactor, and bug fixes at once is a PR that can’t be reviewed well. Reviewers can’t focus because they must understand too much context at once.

// ✗ A PR mixing too much
PR: "Update payment flow"
  - Add multi-currency feature (300 lines)
  - Refactor the entire payment service (500 lines)
  - Fix timeout bug (20 lines)
  - Update grpc-go dependency (50 lines)
→ Reviewers don't know where to focus
→ Bugs in the new feature hide behind the volume of changes

// ✓ Focused PRs
PR 1: "Add multi-currency support to payment flow" (300 lines)
PR 2: "Fix payment service timeout bug" (20 lines)
PR 3: "Update grpc-go dependency" (50 lines)
→ Each PR can be reviewed independently
→ Bugs are easier to detect
→ If something goes wrong, easy to roll back per PR

A Reviewable PR Size #

PRs that are too large won’t get good-quality reviews — reviewers will lose focus or do approval theater (approving without really reading).

PR size targets:
  ✓ < 300 lines: ideal, reviewable in 30-45 minutes
  ⚠ 300-600 lines: possible, but needs a focused reviewer
  ✗ > 600 lines: very difficult to review well

If a PR must be large (e.g. an unbreakable refactor):
  ✓ Explain its structure in the description — what to read first
  ✓ Use PR comments to point out the most important parts
  ✓ Consider a separate review meeting for very large PRs

The PR Description Is Part of the Code #

A good description saves reviewer time more than anything else. Reviewers who understand the PR context give more relevant, on-target feedback.

// ✗ An unhelpful description
Title: "Fix bug"
Description: "Fixed the issue"

// ✓ A description giving full context
Title: "Fix race condition on concurrent order status update"

## Background
There's a race condition when two requests update the same order status
concurrently. This causes an order status to revert to a previous state
if both requests succeed but with a different commit order than expected.

## Changes
- Add optimistic locking using a version field on the orders table
- Update the repository layer to detect concurrent updates and return a clear error
- Add retry logic in the service layer for version conflict cases

## Impact
- The PATCH /orders/{id}/status endpoint now handles concurrent requests safely
- No breaking changes to the API contract
- Performance not significantly affected (benchmark attached)

## How to Test
1. Run `go test ./internal/order/...`
2. For concurrent tests: `go test -race ./internal/order/...`
3. Manual test: open two tabs, update the same order status simultaneously

Self-Review Before Asking for Review #

Engineers who review their own code before opening a PR for others will find most of the comments that would have come from the reviewer — and save everyone time.

Self-review checklist before opening a PR:
  □ Read the entire diff from the perspective of a reviewer who lacks context
  □ Make sure there's no debug code, console.log, or temporary comments
  □ Make sure variable and function names are clear and consistent
  □ Make sure there's no commented-out code without explanation
  □ Make sure there are no forgotten TODOs without a related ticket
  □ Make sure CI (tests, lint, build) passed before opening the PR

Fundamental Code Review Principles (Reviewer Side) #

Prioritize What Matters Most #

Not all code review comments carry equal weight. Reviewers who don’t prioritize often spend time on minor things while missing more critical problems.

Code review priorities (most important first):
  1. Correctness: does this code do what it's supposed to?
     → Logic bugs, off-by-one errors, null pointers, race conditions
  2. Security: is any vulnerability exposed?
     → Input validation, SQL injection, exposed credentials, insecure random
  3. Design & Architecture: does this match the agreed architecture?
     → Layer boundary violations, unnecessary coupling, RFC violations
  4. Performance: are there significant performance problems?
     → N+1 queries, unnecessary loops, memory leaks
  5. Readability: is the code easy to understand?
     → Confusing names, overly long functions, unclear logic
  6. Style: does it match team coding conventions?
     → Lowest priority — much of this can be handled by linters

// ✗ A reviewer with wrong priorities
Spending 20 minutes commenting on naming conventions
Missing the logic bug in a more complex function

// ✓ A reviewer with right priorities
Going straight to the most critical logic first
Only then the more minor things after the important ones are discussed

Give Context to Every Comment #

Comments without context don’t help the author understand why a change is needed. They also hinder meaningful discussion.

// ✗ Comments without context

"This is wrong"

"Use a different approach"

"This isn't good"

// ✓ Comments with clear context

"This approach will cause an N+1 query when `orders` has many items.
 Consider using eager loading with a JOIN or preload:
 `db.Preload("Items").Find(&orders)`
 This reduces the number of queries from O(n) to O(1)."


"This function does too many things (fetch data, transform, validate, save).
 Per SRP, consider splitting it into several more focused functions.
 This will also make unit testing easier."

Distinguish Comment Types #

Not all code review comments have the same importance. Labeling comments helps authors understand which must be addressed and which are just suggestions.

// Common labeling conventions

[BLOCKING] — Must be fixed before the PR can merge

"[BLOCKING] There's potential SQL injection in this query because the input isn't sanitized."

[SUGGESTION] — Recommended but not mandatory

"[SUGGESTION] Consider using a constant for this magic number
 so it's easier to understand."

[NITPICK] — Minor, no impact on logic or maintainability

"[NITPICK] Typo in the comment: 'recieve' → 'receive'"

[QUESTION] — Needs clarification from the author

"[QUESTION] Why do we use a 30-second timeout here?
 Is there specific context for this number?"

[FYI] — Information that may be useful, no action needed

"[FYI] There's a new library that could simplify this, but no need to change it now."

Review in a Timely Manner #

PRs left waiting too long for review are one of the biggest frustration sources in engineering. Authors lose context because they’ve moved to other tasks, and PRs risk merge conflicts.

// Common review SLAs
  ✓ Normal PR (< 300 lines): first review within 4 working hours
  ✓ Large PR (> 300 lines): first review within 1 working day
  ✓ Urgent PR/hotfix: review within 1 working hour
  ✗ PRs left more than 2 working days without any response

If you can't give a full review, a useful minimal response:
  "I'll review this PR tomorrow morning — I have another deadline today"
  "I skimmed it, there's one thing I want to discuss first: [question]"

PR and Code Review Anti-Patterns to Avoid #

// ✗ Monster PRs — too large to review well
A PR with 2000+ lines of changes, mixing features, refactors, and bug fixes
→ Reviewers can't give quality reviews
→ Bugs hide behind the volume of changes
// ✓ Split into small PRs focused on one purpose

// ✗ LGTM without reading
Reviewer approves a 500-line PR in 2 minutes
→ Code review becomes a formality
→ Preventable bugs slip into production
// ✓ If you don't have review time, say when you can — don't approve blindly

// ✗ Passive-aggressive or personal attacks

"This is clearly wrong. Did you even read the documentation?"

"This approach shows you don't understand how Go works"
// ✓ Focus on code: "This approach isn't idiomatic Go because..."

// ✗ PRs without a description or with an unhelpful one
Title: "fix"
Description: (empty)
// ✓ Descriptions contain background, main changes, impact, and how to test

// ✗ Unclear, non-actionable comments

"Refactor this"

"This isn't good"
// ✓ Explain why and offer an alternative approach

// ✗ Opening PRs without passing CI
Opening a PR with 5 failing tests "to be fixed later"
→ Reviewers waste time on a PR that isn't ready
// ✓ PRs must not be opened for review if CI hasn't passed

// ✗ Reviews focusing only on style and ignoring logic
Reviewer gives 10 comments on naming and formatting
but doesn't read the function with the logic bug
// ✓ Prioritize correctness and design over style

Review-Ready PR Checklist #

AUTHOR — Before opening the PR for review:
  □ One PR, one purpose — no mixing features, refactors, and bug fixes
  □ PR size within reviewable limits (< 300 lines if possible)
  □ Self-review done — code read from a reviewer's perspective
  □ PR description includes: background, main changes, impact, how to test
  □ CI passed (tests, lint, build) — nothing failing
  □ No debug code, console.log, or forgotten TODOs
  □ Tests added for new logic
  □ If the PR is large, the description includes a file reading order guide

REVIEWER — During review:
  □ Read the PR description before opening the diff
  □ Understand the PR's purpose before starting to comment
  □ Prioritize: correctness → security → design → performance → readability → style
  □ Every comment includes context and reasoning
  □ Distinguish blocking, suggestion, nitpick, and question
  □ Appreciate the good things, not just criticize the shortcomings
  □ Give the review within the team-agreed SLA
  □ End with a clear decision: Approve, Request Changes, or questions

TEAM — Overall:
  □ An agreed, honored review SLA exists
  □ A PR template exists and is used consistently
  □ CI must pass before a PR can merge
  □ No code can bypass review (branch protection active)

Summary #

• A PR is a social contract, not a formality — the author commits that the code is ready to be accountable for; the reviewer commits to constructive, timely feedback.
• Code is a team asset — engineers who treat their code as personal property get defensive when reviewed; engineers who see it as a shared asset welcome feedback.
• Review code, not people — all feedback is directed at implementation and technical decisions, not the author’s character or ability.
• One PR, one purpose — PRs mixing too much can’t be reviewed well and hide bugs.
• The PR description is as important as the code — reviewers with full context give far more relevant feedback.
• Prioritize correctness and design over style — logic bugs slipping through review are far more dangerous than inconsistent formatting.
• Label your comments — distinguish blocking, suggestion, nitpick, and question so authors can prioritize responses.
• Timely review is a form of respect — PRs waiting too long waste the author’s context and hinder delivery.
• Bugs found at code review are far cheaper than bugs found in production — this is the clearest ROI of a serious PR process.

← Previous: Knowledge Sharing   Next: PR Anatomy →