Code Review Culture
Code review is the team's main teaching loop for Python craft: types, async pitfalls, SQL safety, and test gaps. High-signal culture means clear standards, respectful feedback, and predictable turnaround - not perfectionism that blocks delivery.
Recipe
Quick-reference recipe card - copy-paste ready.
## PR description template
**Intent:** one sentence
**Risk:** migration / perf / security flag
**Test evidence:** pytest path, screenshot, load note
**Rollback:** flag or revert plan
## Review comment tiers
🔴 Must fix - correctness, security, data loss
🟡 Should fix - clarity, missing test (author may defer with ticket)
🟢 Nit - style; author discretionWhen to reach for this:
- Review latency exceeds 24 hours routinely
- Reviews are style wars without teaching
- Juniors fear posting PRs
- Incidents trace to missed review checks
Working Example
# Author addresses review with explicit trade-off comment
# REVIEW: kept sync ORM here intentionally - RFC-042 phase 1 read-only.
# Async migration tracked in ENG-4412.
def get_catalog_item(item_id: str) -> dict[str, object]:
...## Reviewer checklist (Python)
- [ ] Types on public functions / Pydantic models validated
- [ ] No blocking I/O in async def without asyncio.to_thread
- [ ] DB queries parameterized; no string SQL concat
- [ ] Tests cover happy path + one failure mode
- [ ] Logging structured; no PII in messagesWhat this demonstrates:
- PR intent and risk visible before diff read
- Comment tiers separate blockers from nits
- Deferred work links to ticket, not endless review loops
- Python-specific checklist catches fleet-wide gotchas
Deep Dive
How It Works
- Author responsibility - Small diffs, self-review, tests run locally with
uv run pytest. - Reviewer responsibility - Respond within SLA; ask questions before prescribing rewrites.
- Teaching - Link to internal doc or SME page when suggesting pattern.
- Rotation - Spread domain knowledge; avoid single gatekeeper.
- Automation - Ruff, mypy/pyright, and security scanners handle mechanical issues.
Review SLA Guidelines
| PR size | Target first response |
|---|---|
| < 200 LOC | Same business day |
| 200-500 LOC | 24 hours |
| > 500 LOC | Author should split |
Python Notes
# pyproject.toml - let CI enforce style so review focuses on design
[tool.ruff]
line-length = 100
select = ["E", "F", "I", "UP", "B", "ASYNC"]Gotchas
- LGTM without reading tests - Regressions ship. Fix: Require test evidence in PR template.
- Nit comments as blockers - Morale and velocity die. Fix: Use 🟢 tier; batch nits once.
- Giant PRs - Nobody reviews deeply. Fix: Split by feature flag or vertical slice.
- Reviewer rewrites entire patch - Author learns little. Fix: Suggest direction + link to example.
- Only seniors review - Bus factor grows. Fix: Pair junior reviewer with senior on large changes.
Alternatives
| Alternative | Use When | Don't Use When |
|---|---|---|
| Pair programming | Complex novel code | Simple CRUD with tests |
| Mob review session | Architecture shift | Daily small PRs |
| Auto-approve bots | Docs-only typo | Application logic |
| Formal inspection | Regulated safety | Normal product velocity |
FAQs
How many reviewers required?
One for routine; two for security, migrations, or shared libraries.
Should reviewers run code locally?
For risky changes yes; otherwise trust CI if tests comprehensive.
How handle disagreeing reviews?
Author replies with rationale; escalate to tech lead if stalemate.
Review async migrations specially?
Yes - check loop blocking, pool settings, and rollback flag.
Comments on generated code?
Review generator inputs and templates, not every line of output.
How review notebooks?
Focus on data leakage, non-determinism, and production promotion path.
PR size limits?
Soft cap 400 LOC changed; exceptions need RFC link.
Review own code?
Never sole approver; author can merge after other approval only.
How measure review health?
Time to first review, comment tier ratio, post-merge defect rate.
LLM-assisted review?
Use for checklist pass; human owns security and domain correctness.
Related
- Mentoring & Leveling - reviews as teaching
- Running Design Reviews - before big PRs
- Handling Technical Disagreements - stalemates
- Linting & Formatting - automate nits
- pytest Testing - test expectations
Stack versions: This page was written for Python 3.14.0 (stable 3.14, maintenance 3.13), FastAPI 0.115+, Django 5.2, Flask 3.1, Pydantic 2, PyTorch 2.6+, pandas 2.2+, Polars 1.x, ruff 0.9+, and uv 0.6+.