Writing Good Pull Requests

The PR as a Conversation

A pull request is not a code dump. It is a proposal — a conversation between you and the maintainers about a change to their project. The code is only part of the PR. The description, the commit history, the test coverage, and the way you respond to feedback all determine whether your PR gets merged or closed.

The best PRs are ones where the reviewer can understand the change in five minutes, verify it is correct, and click merge. Every minute the reviewer spends confused is a minute they could spend on another PR, and every additional round of review feedback delays the merge by days.

Small PRs Over Big Ones

The single most important rule for pull requests: keep them small.

A study by SmartBear (based on a Cisco code review study) found that review quality drops sharply after 200-400 lines of code. Beyond 400 lines, reviewers start skimming. Beyond 1,000 lines, they approve without reading carefully because the cognitive load is too high.

Lines Changed    Review Quality
--------------   ----------------
1-100            Thorough, detailed feedback
100-400          Good review, catches most issues
400-1000         Reviewer skims, misses edge cases
1000+            "Looks good to me" (rubber stamp)

The Kubernetes project enforces this culturally. Large PRs are routinely sent back with a request to split them. The Go project's contribution guide explicitly asks for small, focused changes. React's core team has said that the PRs most likely to be merged are small ones with clear descriptions.

How to Split Large Changes

If your change requires touching many files, break it into a sequence of PRs:

Refactoring PR: Move code around, extract functions, rename variables. No behavior change.
Infrastructure PR: Add new types, interfaces, or database tables. No behavior change yet.
Feature PR: Implement the feature using the infrastructure from step 2.
Cleanup PR: Remove old code paths, update documentation.

Each PR is independently reviewable, independently mergeable, and independently revertable. If the feature PR introduces a bug, you can revert it without losing the refactoring work.

Writing the Description

The PR description is the most underrated part of the contribution. A good description answers three questions:

What Changed?

Describe the change concretely. "Updated the authentication module" is vague. "Added rate limiting to the login endpoint: 5 attempts per minute per IP, returning HTTP 429 after the limit is exceeded" is specific.

Why?

Link to the issue, bug report, or discussion that motivated the change. If there is no issue, explain the problem you are solving. "Users reported that brute-force login attacks were succeeding because there was no rate limiting (issue #1234)" gives the reviewer context they need.

How?

For non-trivial changes, explain the approach. If you chose one design over another, explain why. This saves the reviewer from guessing at your reasoning and suggesting the approach you already considered and rejected.

## What
Added rate limiting to the login endpoint using a sliding window
counter stored in Redis.

## Why
Issue #1234: Users reported brute-force attacks succeeding against
accounts with weak passwords. Without rate limiting, an attacker
can try thousands of passwords per minute.

## How
- Used a sliding window counter (not fixed window) to prevent
  burst attacks at window boundaries
- Chose Redis over in-memory storage because the API runs on
  multiple instances behind a load balancer
- Set the limit to 5 attempts per minute per IP, returning
  HTTP 429 with a Retry-After header
- Added bypass for health check endpoints

## Testing
- Unit tests for the rate limiter logic
- Integration test with a test Redis instance
- Manual testing with curl to verify 429 responses

Link to the Issue

If the change addresses an issue, link it. Most platforms support closing issues automatically: "Fixes #1234" or "Closes #1234" in the PR description will close the issue when the PR is merged. This keeps the issue tracker clean and provides traceability.

Following the Project's Style

Every project has conventions, and violating them creates friction:

Code Style

If the project uses tabs, use tabs. If they use 2-space indentation, use 2-space indentation. If they use camelCase for variables, do not introduce snake_case. Run the project's formatter before committing. Most modern projects enforce formatting in CI (Prettier for JavaScript, rustfmt for Rust, Black for Python, gofmt for Go), but do not rely on that — format locally.

Commit Messages

Some projects follow Conventional Commits (feat:, fix:, docs:). Some use a specific format like the Linux kernel style (imperative mood, 50-character subject, body wrapped at 72 characters). Some just want clear, descriptive messages. Check recent commits to understand the convention:

// Linux kernel style
Fix null pointer dereference in network driver

The rx_handler function did not check for null before
dereferencing the sk_buff pointer, causing a kernel panic
when receiving malformed packets.

Signed-off-by: Name <email>

// Conventional Commits style
fix(auth): prevent timing attack on password comparison

Replaced == with constant-time comparison for password
hash verification.

Closes #1234

Test Style

If the project uses a specific test framework, assertion style, or test organization pattern, follow it. If tests are organized by feature, do not create a new "misc_tests" file. If they use behavior-driven naming (it_should_return_error_when_input_is_empty), do not name your test test1.

Writing Tests for Your Change

PRs without tests are incomplete. The reviewer has to trust that your code works based on reading it, which is unreliable. Tests prove that the change works and protect against regressions.

What to Test

New features: Test the happy path and at least two edge cases
Bug fixes: Write a test that reproduces the bug (fails before your fix, passes after)
Refactoring: Existing tests should still pass. If they do not, your refactoring changed behavior

The Regression Test

For bug fixes, the most valuable test is one that reproduces the original bug:

TEST issue_1234_empty_input_does_not_crash
    // This input caused a panic before the fix
    result ← PROCESS_INPUT("")
    ASSERT result IS error
    ASSERT result.error = InputError::Empty

This test serves as documentation of the bug and a guarantee that it will never regress. The Rust project requires regression tests for every bug fix merged into the compiler.

Responding to Review Feedback

Respond Promptly

When a reviewer leaves comments, respond within a day or two. If you cannot address the feedback immediately, reply with "Thanks for the review, I'll address these comments this weekend." Unresponsive PRs get closed.

Push Fixes as New Commits

When addressing review feedback, push fixes as new commits rather than force-pushing over the existing history. This lets the reviewer see exactly what changed since their last review. Many projects squash commits on merge, so the intermediate commits will not pollute the main branch history.

INITIAL PR:
    commit: "Add rate limiting to login endpoint"

AFTER FIRST REVIEW:
    commit: "Add rate limiting to login endpoint"
    commit: "Address review: use constant-time comparison"
    commit: "Address review: add Retry-After header"

ON MERGE (squashed by maintainer):
    commit: "Add rate limiting to login endpoint (#1234)"

Disagree Respectfully

If you disagree with review feedback, explain your reasoning clearly. "I considered that approach, but chose this one because X" is productive. "That's wrong" is not. Sometimes the reviewer is right and you need to change your approach. Sometimes you are right and they need more context. Either way, the conversation should be professional and focused on the code.

Do Not Take Rejection Personally

Maintainers reject PRs for many reasons:

The feature does not align with the project's direction
The implementation approach conflicts with planned changes
The change introduces complexity that outweighs the benefit
The same issue was already fixed in another branch
The maintainer simply does not have time to review a large change

None of these are personal. The React team routinely closes feature requests with detailed explanations of why the feature does not fit React's philosophy. The Go team rejects proposals that add complexity without sufficient justification. This is how healthy projects maintain focus and quality.

If your PR is rejected, ask for feedback. "What would make this change acceptable?" or "Is there a different approach that would work?" turns a rejection into a learning opportunity.

The Checklist Before Submitting

Before clicking "Create pull request," verify:

PRE-SUBMISSION CHECKLIST

[ ] Read the CONTRIBUTING.md and followed its guidelines
[ ] Created a focused branch with a descriptive name
[ ] Made a small, focused change (under 400 lines if possible)
[ ] Ran the test suite locally and all tests pass
[ ] Ran the linter/formatter and fixed all warnings
[ ] Wrote tests for new functionality or bug fixes
[ ] Wrote a clear PR description (what, why, how)
[ ] Linked to the relevant issue
[ ] Reviewed my own diff before submitting

That last item — reviewing your own diff — catches embarrassing mistakes. Leftover debug print statements, commented-out code, unrelated formatting changes. Spend two minutes reviewing your diff on GitHub before asking someone else to review it.

Keep PRs small (under 400 lines). Review quality drops sharply with size, and large PRs sit unreviewed for longer.
Write clear descriptions covering what changed, why, and how. Link to the relevant issue.
Follow the project's style guide for code, commits, and tests. Consistency matters more than personal preference.
Include tests for every feature and bug fix. A PR without tests is incomplete.
Respond to review feedback promptly. Push fixes as new commits so reviewers can see the delta.
Do not take rejection personally. Ask for feedback and learn from it.