Python Code Review
Review Python code changes systematically using established best practices and checklists.
Review Workflow
1. Gather Context
# Get the diff to review
git diff main...HEAD
# Or for a specific PR
gh pr diff <pr-number>
2. Run Automated Checks First
Before manual review, verify automated tools pass:
ruff check . # Linting
ruff format --check . # Formatting
mypy src/ # Type checking
pytest tests/ -v # Tests
3. Systematic Code Review
Review each file in the diff, checking these categories in order:
Security (Critical)
- No SQL string formatting (use parameterized queries)
- No
eval()/exec()with user input - No hardcoded secrets
- Inputs validated at boundaries
- No path traversal vulnerabilities
Correctness
- Logic handles edge cases (empty, None, boundaries)
- Error handling is specific (no bare
except:) - Exception chains preserved (
raise ... from e) - No mutable default arguments
- Types are correct and complete
Design
- Functions do one thing (<30 lines)
- Classes follow single responsibility
- No code duplication (DRY)
- Appropriate data structures used
- No premature optimization
Style
- Naming is clear and consistent (snake_case, PascalCase)
- No magic numbers/strings
- Docstrings on public API
- No commented-out code
4. Provide Feedback
Structure feedback by severity:
- Blockers - Must fix before merge (security, bugs, breaking changes)
- Suggestions - Should consider (design, performance, readability)
- Nits - Minor style issues (optional to fix)
Red Flags to Catch
| Pattern | Issue |
|---|---|
except: or except Exception: | Catches too much, hides bugs |
def foo(items=[]) | Mutable default argument bug |
f"SELECT * FROM {table}" | SQL injection |
eval(user_input) | Code injection |
password = "secret123" | Hardcoded secret |
| No type hints on public functions | Harder to maintain |
| Functions >50 lines | Likely doing too much |
# TODO without ticket | Tech debt without tracking |
Common Patterns to Suggest
# Instead of bare except
try:
process()
except: # Bad
pass
# Use specific exceptions
try:
process()
except ValueError as e:
logger.exception("Processing failed")
raise
# Instead of mutable defaults
def add_item(item, items=[]): # Bad
items.append(item)
# Use None pattern
def add_item(item, items: list | None = None) -> list:
if items is None:
items = []
items.append(item)
return items
# Instead of verbose loops
result = []
for x in data:
if x.valid:
result.append(x.value)
# Use comprehensions
result = [x.value for x in data if x.valid]
Reference Guides
For detailed guidelines, consult:
- Checklist - Complete review checklist with checkboxes
- Best Practices - Comprehensive Python best practices
Use these references when:
- Explaining why something should change
- Providing examples of better patterns
- Checking specific categories in depth
