Skip to content

Pull Requests

Intuition

Section titled “Intuition”

A formal code review process: Pull requests are like submitting a proposal to a committee, you present your changes, colleagues review them, discuss improvements, and only then does the change get approved and merged. It turns solo coding into a team activity.

Why it matters: PRs are the gatekeeper of code quality. They catch bugs before they reach production, spread knowledge across the team, and create a permanent record of why changes were made.

The key insight: Good PR descriptions save hours of review time, explaining what changed, why it changed, and how to test it transforms a review from guesswork into a focused discussion.

What is a Pull Request

Section titled “What is a Pull Request”

A pull request (PR), called a merge request in GitLab, is a proposal to merge a branch into another branch, accompanied by a code review discussion. While git merge is a local operation, a pull request is a platform feature (GitHub, GitLab, Gitea) that adds:

  • A web interface for reviewing diffs.
  • Discussion threads on specific lines of code.
  • Automated CI checks.
  • Approval workflows.
  • Audit trail of who reviewed and when.

Anatomy of a Good Pull Request

Section titled “Anatomy of a Good Pull Request”

Title

Section titled “Title”

The title should be a concise summary of the change:

feat(auth): add JWT token refresh mechanism
fix(parser): handle null input without segfault
refactor(api): extract validation into separate module
docs(readme): update installation instructions for macOS

Description

Section titled “Description”

A good PR description answers:

  1. What does this PR change?
  2. Why is this change needed? (Context, motivation, issue reference)
  3. How was it implemented? (Design decisions, alternatives considered)
  4. How to test? (Steps for the reviewer to verify)

Template:

```mermaid
flowchart TD
    A[03 Pull Requests] --> B[Key Concepts]
    A --> C[Core Principles]
    A --> D[Practical Applications]
    B --> E[Fundamental definitions]
    C --> F[Design patterns]
    D --> G[Real-world usage]

Summary

Section titled “Summary”

Brief description of the change.

Motivation

Section titled “Motivation”

Why this change is needed. Reference to issue: Closes #42.

Changes

Section titled “Changes”
  • Added JWT token refresh middleware
  • Updated token expiry from 1h to 15min
  • Added refresh token rotation

Testing

Section titled “Testing”
  1. Login with valid credentials
  2. Wait for access token to expire (or reduce expiry for testing)
  3. Verify that requests succeed (token was automatically refreshed)
  4. Verify that refresh token was rotated

Screenshots

Section titled “Screenshots”

(If applicable)

Checklist

Section titled “Checklist”
  • Tests pass
  • No linting errors
  • Documentation updated
  • Breaking changes documented
### Size


Small PRs are easier to review, faster to merge, and safer to revert. Aim for:


| Metric        | Target   | Maximum  |
| ------------- | -------- | -------- |
| Lines changed | < 200    | < 400    |
| Files changed | < 10     | < 20     |
| Review time   | < 30 min | < 1 hour |


If a PR exceeds these limits, consider splitting it into multiple smaller PRs.


## Code Review


### Reviewing PRs


When reviewing a PR, focus on:


| Category        | What to Check                                                                           |
| --------------- | --------------------------------------------------------------------------------------- |
| **Correctness** | Does the code do what the PR claims? Are there edge cases?                              |
| **Design**      | Does the change fit the existing architecture? Is the abstraction appropriate?          |
| **Readability** | Is the code understandable? Are variable/function names clear?                          |
| **Testing**     | Are there tests for the new behavior? Do existing tests still pass?                     |
| **Security**    | Does the change introduce any vulnerabilities? (Input validation, authentication, etc.) |
| **Performance** | Does the change introduce any regressions? (N+1 queries, unnecessary copies, etc.)      |


### Writing Review Comments


**Be constructive and specific:**


| Bad                      | Good                                                                   |
| ------------------------ | ---------------------------------------------------------------------- |
| "This is wrong"          | "This condition is inverted, `x < 0` should be `x >= 0`"              |
| "Use a better algorithm" | "A binary search would reduce this from $O(n)$ to $O(\log n)$"         |
| "This is hard to read"   | "Extract this into a named function `is_valid_email` with a docstring" |


**Distinguish blocking from non-blocking comments:**


```markdown
[REQUIRED] This must be fixed before merging.


- The null check is missing -- this will crash if `user` is null.


[SUGGESTION] Consider this improvement (non-blocking).


- You could use `String::from_utf8_lossy` instead of `unsafe { String::from_utf8_unchecked }`.


[QUESTION] I"m not sure about this -- please clarify.


- Why is the timeout set to 30 seconds? Is this documented somewhere?

CI Integration

Section titled “CI Integration”

Required Status Checks

Section titled “Required Status Checks”

Configure GitHub to require certain checks before a PR can be merged:

  1. Tests: npm test``cargo test``pytestEtc.
  2. Linting: eslint``clang-tidy``flake8Etc.
  3. Type checking: tsc --noEmit``mypyEtc.
  4. Build: npm run build``cargo buildEtc.
  5. Security: npm audit``snyk testEtc.

Branch Protection Rules

Section titled “Branch Protection Rules”

For the main branch, configure:

RulePurpose
Require PR before mergingAll changes must be reviewed
Require approvals (1+)At least one person must approve
Require status checksAll CI checks must pass
Require up-to-date branchPR must be rebased on latest main before merging
Require signed commitsCommits must be GPG/SSH signed
Require linear historyNo merge commits (force rebase)
Restrict who can pushOnly authorized users can bypass PRs

Auto-Merge

Section titled “Auto-Merge”

After all checks pass and approvals are granted, enable auto-merge:

Terminal window
## Via GitHub CLI
$ gh pr merge 42 --auto --squash

Merging Strategies on Platforms

Section titled “Merging Strategies on Platforms”
StrategyGit CommandWhen to Use
Create a merge commitgit merge --no-ffPreserves full history; good for large teams
Squash and mergegit merge --squashLinear history; one commit per PR
Rebase and mergegit rebase + git merge --ff-onlyLinear history; preserves individual commits

Squash and Merge: Implications

Section titled “Squash and Merge: Implications”

Squash and merge creates a single commit from all commits in the PR. This has consequences:

  • Pros: Clean, linear history; easy to git bisect; PR is a single unit.
  • Cons: Individual commit messages are lost; cannot revert a specific commit within the PR.

Cross-References

Section titled “Cross-References”
  • Remote Operations provides the push and fetch mechanics that underpin pull request creation and merging.
  • Workflows defines the team conventions within which pull requests operate as the review mechanism.
  • Merging covers the merge strategies used when integrating pull request branches.