Recap — Good PRs and Good Reviews

# Good PRs and Good Reviews Pull requests are how code joins the main branch in modern dev teams. Doing them well makes everyone faster. ## The PR lifecycle ``` 1. Create a branch → git switch -c feature/login 2. Make changes, commit → work, commit, commit 3. Push to remote → git push -u origin feature/login 4. Open a PR on GitHub → "compare changes" → "open pull request" 5. Reviewers comment → discuss, request changes 6. Make updates → more commits OR amend + force-push 7. Get approval → reviewer clicks "approve" 8. Merge → merge button on GitHub 9. Delete branch → cleanup ``` ## What makes a good PR ### Small Aim for <400 lines changed. Big PRs get superficial reviews; small ones get careful ones. If a feature is big, split into multiple PRs: - "Refactor X" → review. - "Add new behavior on top of refactored X" → review. - "Wire up UI" → review. Each is reviewable in 20 minutes. ### Single-purpose One PR = one logical change. "Add login + fix typo + refactor users" = three PRs. Why: easier to review, easier to revert, clearer history. ### Good description ```markdown ## Summary Adds password reset flow via email link. ## What changed - New endpoint `/api/auth/reset-password` - Email template for reset link - Token validation logic (1-hour expiry) - Tests for happy path + expired token ## Why Closes #234. Users have requested for months. ## How to test 1. POST /api/auth/reset-password with valid email 2. Check email for link 3. Click link; should redirect to set-password page 4. Submit new password; should log in ## Screenshots [before/after of UI if applicable] ``` Tells the reviewer: - What this does. - Why it exists. - How to verify it works. - Context (linked issue). ### Tests Most PRs should include tests for the new behavior. CI runs them; reviewer sees pass/fail. ### Clean history Before opening PR, consider squashing WIP commits via interactive rebase. Each commit on the branch should be meaningful. ## Reviewing PRs — the giver's side ### Read the description first The PR's why and what before diving into code. Sets context. ### Look at the test plan If "how to test" exists, mentally walk through it. Does the code support it? ### Look at the tests Tests reveal intent. "What is this code MEANT to do?" answered by tests. ### Then code, top-down Start from public interfaces (the new endpoints, classes, types). Then internals. ### Comment styles ``` - [nit] Minor style suggestion; not blocking. - [suggest] Better approach but yours works. - [question] Genuinely curious; can you explain? - [must-fix] Blocking; must change before merge. - [praise] Good pattern; worth calling out. ``` Marking comments with tags makes intent clear. Reviewers and authors save time. ### Be kind Code review can feel personal. Some defaults: - Critique code, not the person. - Ask questions rather than dictate. - Propose, don't impose. - Acknowledge good work. ### Review scope Don't ask for the moon. The PR has one purpose; comments outside that scope are usually "out of scope for this PR" — file a separate issue/PR. ## Responding to reviews — the receiver's side ### Treat feedback as information It's not personal. Someone is trying to improve the code. Even if you disagree, take a beat before responding. ### Respond to each comment Either: - Make the change (and resolve the comment). - Push back with reasoning. - Ask for clarification. Don't ignore comments. ### Address [must-fix] before re-requesting review Tag the reviewer or use GitHub's "Re-request review" button when ready. ### Discuss heated topics offline If a comment thread is growing argumentative, take it to a call/chat. Saves time and tone. ## Merging strategies GitHub offers three: ### Merge commit (default) Creates a merge commit with all PR's commits. History shows the PR as a branch. ### Squash and merge Combines all PR commits into ONE on main. Clean linear history; loses commit-level detail. ### Rebase and merge Replays PR commits onto main individually. Linear history with separate commits. Team picks one (often configurable per repo). Common choice: **Squash and merge** for clean main + PR description as the commit message. ## Draft PRs ``` Mark PR as draft until ready for review. ``` Use for: WIP that you want early feedback on but isn't ready to merge. Or for stacked PRs awaiting upstream merge. ## CODEOWNERS A `.github/CODEOWNERS` file maps paths to required reviewers: ``` /backend/ @backend-team /frontend/ @frontend-team *.yml @devops ``` Auto-requests review from owners when PRs touch those paths. ## PR templates `.github/pull_request_template.md` pre-populates PR descriptions with sections you require: ```markdown ## Summary ## Changes ## Testing ## Related Issues ``` Helps consistency. ## Protected branches Repo settings → Branches: - Require PR for changes to main. - Require N approvals. - Requir