Pull Requests — Professional Code Review
Module 04 75 min
Section Objectives
- Create complete, professional Pull Requests
- Perform quality code reviews
- Manage the PR lifecycle (creation → review → merge)
- Configure protected branches and required checks
What is a Pull Request?
A Pull Request (PR) is a proposal to integrate changes from one branch into another. It's not a Git feature — it's a GitHub (and GitLab/Bitbucket) feature.
Why Pull Requests?
| Without PRs | With PRs |
|---|---|
| Direct push to main | Code reviewed before merging |
| Bugs reach production | Bugs caught during review |
| No knowledge sharing | Team learns from reviews |
| No audit trail | Full history of decisions |
| Individual work | Collaborative work |
Creating a Quality Pull Request
PR Title
# Format: type(scope): short description
feat(auth): add JWT authentication system
fix(api): handle 404 for unknown users
docs(readme): add deployment instructions
refactor(cart): extract price calculation logic
PR Description Template
## Summary
Brief description of changes and their purpose.
## Changes Made
- Added `JWTService` class for token generation/validation
- Updated `UserController` to use JWT on all protected endpoints
- Added middleware `auth_required` for route protection
- Added JWT configuration to `settings.py`
## Why These Changes?
The old session-based auth didn't scale for our mobile API.
JWT enables stateless authentication, reducing server load by ~40%.
## Screenshots (if applicable)
[Add before/after screenshots for UI changes]
## Type of Change
- [x] New feature
- [ ] Bug fix
- [ ] Breaking change
- [ ] Documentation update
## Tests
- [x] Unit tests added/updated
- [x] Integration tests pass
- [x] Manually tested on staging
## Checklist
- [x] Code follows project conventions
- [x] Self-review done
- [x] Comments added for complex code
- [x] Documentation updated
- [x] No new linting warnings
## Related Issues
Closes #78
Related to #65
Creating a PR via CLI
# Simple PR
gh pr create --title "feat(auth): add JWT" --body "See description"
# PR with full template
gh pr create \
--title "feat(auth): add JWT authentication" \
--body "$(cat .github/PULL_REQUEST_TEMPLATE.md)" \
--base main \
--head feature/jwt-auth \
--reviewer alice,bob \
--label "feature,authentication" \
--assignee @me
Performing a Code Review
Review Best Practices
| Good Practice | Description |
|---|---|
| Review the diff first | Get the big picture before details |
| Understand the context | Read the issue and PR description first |
| Be specific | Point to exact lines, not vague feedback |
| Explain the why | "This could cause X because..." not just "change this" |
| Distinguish blocking/non-blocking | Distinguish "must change" from "could improve" |
| Acknowledge the good | Positive feedback is also valuable |
| Don't be personal | Comment on code, not on the person |
Types of GitHub Review Comments
# Blocking comment (must fix before merge)
**[BLOCKING]** This function doesn't validate user input.
An attacker could pass arbitrary SQL here.
See: https://owasp.org/sql-injection
# Non-blocking suggestion
**[NIT]** Minor style: we usually name variables in snake_case
in this project. Could rename `userCount` to `user_count`.
# Question (doesn't block)
**[QUESTION]** Why `timeout=30` instead of using the constant
`DEFAULT_TIMEOUT`? Is there a specific reason for this endpoint?
# Positive feedback
**[NICE]** Elegant use of the Strategy pattern here!
This makes adding new payment methods much simpler.
The 3 Types of GitHub Reviews
| Type | Description | When to Use |
|---|---|---|
| Comment | Leaves a comment without decision | Questions, observations |
| Approve | Approves the PR | Code is ready to merge |
| Request changes | Blocks the merge | Required changes |
GitHub Review Interface
View Files Changed
# In terminal, view PR diff
gh pr diff 42
# View a specific file changed
gh pr diff 42 -- src/auth/jwt.py
Respond to Review Comments
# Update your branch after review
git switch feature/jwt-auth
# Make the requested changes
# ...
git add .
git commit -m "fix: address code review feedback
- Add input validation to login endpoint
- Use DEFAULT_TIMEOUT constant instead of magic number
- Rename userCount to user_count"
git push
# GitHub automatically marks conversations as resolved
Protected Branches
Protected branches prevent direct pushes and require PRs with reviews:
Configuring on GitHub
- Go to Settings → Branches
- Click Add rule next to the branch to protect
- Configure the rules:
Branch name pattern: main
✅ Require a pull request before merging
✅ Require approvals: 2
✅ Dismiss stale PR approvals when new commits are pushed
✅ Require review from code owners
✅ Require status checks to pass before merging
✅ Require branches to be up to date before merging
Status checks: [ci/tests, ci/linting]
✅ Require conversation resolution before merging
✅ Require signed commits
✅ Include administrators
❌ Allow force pushes (NEVER on main)
❌ Allow deletions
CODEOWNERS File
# .github/CODEOWNERS
# Define who must review changes to specific files
# Global owners
* @tech-lead @senior-dev
# Backend owners
/src/api/ @backend-team
/src/models/ @backend-team @data-team
# Frontend owners
/src/components/ @frontend-team
/src/styles/ @frontend-team
# DevOps
/.github/ @devops-team
/docker/ @devops-team
/kubernetes/ @devops-team
# Security-sensitive files
/src/auth/ @security-team @tech-lead
/config/ @security-team @tech-lead
PR Merge Strategies
| Strategy | Description | When to Use |
|---|---|---|
| Merge commit | Creates a merge commit, preserves all history | Traceability needed |
| Squash and merge | Condenses all commits into one | Messy history, clean main desired |
| Rebase and merge | Replays commits linearly, no merge commit | Clean linear history |
# Via CLI
gh pr merge 42 --merge # Merge commit
gh pr merge 42 --squash # Squash and merge
gh pr merge 42 --rebase # Rebase and merge
# With auto-delete of branch
gh pr merge 42 --squash --delete-branch
PR Labels and Automation
# .github/labeler.yml — Auto-label PRs based on files changed
documentation:
- '**/*.md'
- 'docs/**'
frontend:
- 'src/components/**'
- 'src/styles/**'
- '**/*.tsx'
- '**/*.css'
backend:
- 'src/api/**'
- 'src/models/**'
- '**/*.py'
devops:
- '.github/**'
- 'docker/**'
- 'kubernetes/**'
- 'Dockerfile'
Summary
| Action | Command/Location |
|---|---|
| Create PR | gh pr create or GitHub UI |
| View PRs | gh pr list |
| Review a PR | gh pr review 42 --approve |
| Merge a PR | gh pr merge 42 --squash |
| Close a PR | gh pr close 42 |
| View PR diff | gh pr diff 42 |