Code Review Guidelines
What reviewers look for in Python pull requests - correctness, tests, security, operability, and team conventions - without turning review into style nitpicks ruff already enforces.
//
Search across all documentation pages
What reviewers look for in Python pull requests - correctness, tests, security, operability, and team conventions - without turning review into style nitpicks ruff already enforces.
Reviewer 5-minute pass:
When to reach for this:
Good PR description:
## [BILL-91] Apply VAT to EU orders
- Adds TaxService with Decimal math
- Alembic revision `a1b2c3` adds `vat_amount` column (nullable, backfill in follow-up)
- Feature flag `billing.vat_enabled` default off
## Test plan
- [x] uv run pytest tests/billing/test_tax.py
- [x] Manual: create order DE address, flag on, see VAT line
## Rollback
Disable flag; migration reversible via downgrade (drops column - OK pre-prod)Good review comment:
**Question:** What happens for zero-quantity line items - divide by zero or skip?
Suggest asserting in `test_tax_zero_quantity` - happy to approve after that.What this demonstrates:
| Priority | Check |
|---|---|
| P0 | Correctness, security, data loss risk |
| P1 | Tests, migrations, API contract breaks |
| P2 | Performance, observability (logs/metrics) |
| P3 | Style (if CI missed), naming, docs |
async def?| Alternative | Use When | Don't Use When |
|---|---|---|
| Pair programming | Complex risky change | Async review timezone spread |
| AI review bot | First pass nitpicks | Sole reviewer for security |
| Design doc before code | Large architecture shift | One-line bugfix |
| No review (solo) | Prototype throwaway | Production main |
Target first response < 1 business day; SLA per team wiki.
Yes if nits are non-blocking and tracked in follow-up ticket optional.
Request changes when merge would cause prod risk; comment for educational or optional.
Encouraged - questions improve docs; senior still CODEOWNER on critical paths.
OK for dependabot patch bumps with policy; not for migrations without human.
Prefer paired .py from Jupytext or review outputs-stripped diff only.
Auth, crypto, new external input parsers, dependency major bumps - tag security buddy.
Hot path queries, new N+1 ORM loops, large pandas in request path.
Author after approval + green CI unless release manager role for train.
Author watches dashboards 30 min after deploy for their change - note in PR template.
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+.
Reviewed by Chris St. John·Last updated Jul 16, 2026