Pull Request Process

A well-structured pull request (PR) process ensures code quality while keeping development moving efficiently. This guide covers both author and reviewer responsibilities.

Author Guidance

Before Creating a PR

Self-Review Checklist

• Code compiles and runs locally
• All tests pass
• New tests added for new functionality
• No debugging code or commented code
• Code follows team style guide
• Commit messages follow Conventional Commits

Preparation

• Break large changes into smaller PRs
• Ensure branch is up to date with main
• Run linters and formatters
• Review your own diff

Creating the PR

Title

Write a clear, descriptive title:

Good: "Add password reset functionality with email verification"
Bad:  "Updates" or "Fix bug" or "WIP"

Description

Include:

1. What - What does this change do?
2. Why - Why is this change needed?
3. How - How does it work (if not obvious)?
4. Testing - How was it tested?
5. Screenshots - For UI changes

PR Template

## Description
[Describe what this PR does and why]

## Type of Change
- [ ] Bug fix (non-breaking change that fixes an issue)
- [ ] New feature (non-breaking change that adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] Documentation update

## How Has This Been Tested?
[Describe tests run]

## Checklist
- [ ] My code follows the style guidelines
- [ ] I have performed a self-review
- [ ] I have commented hard-to-understand areas
- [ ] I have updated documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests for my changes
- [ ] All tests pass locally

## Screenshots (if applicable)
[Add screenshots for UI changes]

## Related Issues
Closes #[issue number]

Commit Message Conventions

Message Structure

<type>[optional scope]: <description>

[optional body]

[optional footer(s)]

Commit Types

Breaking Changes

Indicate breaking changes by:

• Adding ! after type/scope: feat!: remove deprecated API
• Or using footer: BREAKING CHANGE: description

Breaking changes trigger MAJOR version bump (x.0.0).

Examples

# Simple feature
feat: add user profile export

# Feature with scope
feat(auth): implement OAuth2 login

# Bug fix with issue reference
fix(parser): handle empty input arrays

Closes #123

# Breaking change
feat(api)!: change response format to JSON:API

BREAKING CHANGE: All endpoints now return JSON:API formatted responses.
Migrate clients to expect `data` wrapper object.

# Documentation
docs: update API authentication guide

# Multiple footers
fix: prevent duplicate form submissions

Reviewed-by: Alice
Refs: #456, #789

Commit Message Checklist

• Type accurately describes the change
• Description is imperative mood ("add" not "added")
• Description is concise (50 chars or less preferred)
• Body explains "what" and "why" (not "how")
• Breaking changes are clearly marked
• Related issues are referenced in footer

PR Size Guidelines

Responding to Feedback

Do:

• Respond to all comments
• Ask for clarification if needed
• Make requested changes promptly
• Thank reviewers for their input
• Mark conversations as resolved

Don't:

• Take feedback personally
• Argue without listening
• Ignore comments
• Make unrelated changes

Response Patterns:

"Good catch, fixed!"
"I chose this approach because... does that make sense?"
"Could you explain what you mean by...?"
"I'll create a follow-up issue for that larger refactor"

Reviewer Guidance

Review Mindset

Approach reviews with:

• Curiosity - Why did they make this choice?
• Humility - I might be wrong
• Respect - This is someone's work
• Constructive intent - Help improve, don't criticize

What to Look For

Priority Order:

1. Correctness - Does it work? Does it do what it should?
2. Security - Are there vulnerabilities?
3. Design - Is the architecture appropriate?
4. Readability - Is it maintainable?
5. Style - Does it follow conventions?

Comment Types

Blocking (Must Fix)

"This will cause a null pointer exception when user is undefined.
Please add a null check."

Suggestion (Should Consider)

"Consider using Optional here instead of null checks.
It makes the intent clearer."

Nitpick (Minor)

"Nit: This could be a one-liner with a ternary operator,
but the current version is fine too."

Question (Clarification)

"I'm not sure I understand why we need this cache.
Could you explain the use case?"

Praise (Recognition)

"Great solution! I wouldn't have thought of using
a decorator pattern here."

Writing Good Comments

Be Specific

Bad:  "This is wrong"
Good: "This will fail when the array is empty because
      we access [0] without checking length"

Explain Why

Bad:  "Use const instead of let"
Good: "Use const here since this value is never reassigned.
      It signals intent to future readers."

Suggest Solutions

Bad:  "This is inefficient"
Good: "This is O(n²). Consider using a Set for O(1)
      lookups: `const seen = new Set(items)`"

Be Kind

Bad:  "Why would you do it this way?"
Good: "I'm curious about this approach. Was there a specific
      reason for choosing X over Y?"

Review Turnaround

If you can't review in time:

• Communicate delay
• Suggest another reviewer
• Do a partial review

When to Approve

Approve when:

• Code is correct
• Tests are adequate
• No security issues
• Code is maintainable
• Your comments are addressed or acknowledged

Don't block for:

• Style preferences (use linters)
• Theoretical future issues
• Perfect code (good enough is good enough)

PR Workflow States

Draft PRs

Use for:

• Work in progress
• Early feedback
• CI validation

Stacked PRs

For large changes, stack PRs:

main ← PR1 (foundation) ← PR2 (feature A) ← PR3 (feature B)

Review and merge in order.

Merge Strategies

Squash and Merge

• All commits become one
• Clean history
• Good for feature branches

Merge Commit

• Preserves all commits
• Shows branch history
• Good for long-lived branches

Rebase and Merge

• Linear history
• No merge commits
• Good for small changes

Conflict Resolution

Preventing Conflicts

• Keep branches short-lived
• Update from main frequently
• Communicate about shared files

Resolving Conflicts

1. Update your branch from main
2. Resolve conflicts locally
3. Run tests after resolving
4. Push updated branch
5. Request re-review if significant changes

Automation

Required Checks

Configure required status checks:

• All tests pass
• Linting passes
• Build succeeds
• Security scan clean
• Coverage threshold met

Auto-Merge

Enable auto-merge when:

• All checks pass
• Required approvals obtained
• No conflicts

Related Resources

• Review Checklists
• Team Agreements
• Microsoft Playbook: Pull Requests
• Conventional Commits Specification

Compliance

This section fulfills ISO 13485 requirements for design change control (7.3.7) and record management (4.2.4, 4.2.5), and ISO 27001 requirements for change management (A.8.32) and secure development (A.8.25).

View full compliance matrix