mirror of
https://github.com/Sea-Haven-Industries/engineering-handbook.git
synced 2026-09-30 09:13:14 +00:00
0333e7f8f5
Conventions covering naming, git workflow, commit messages, pull requests, code review, GitHub standards, AWS infrastructure, SAM project layout, and secrets management. Commit messages section adapted from RomuloOliveira/commit-messages-guide (CC-BY-4.0).
47 lines
1.8 KiB
Markdown
47 lines
1.8 KiB
Markdown
# Code Review
|
|
|
|
## Purpose
|
|
|
|
Code review exists to catch defects, share knowledge, and maintain consistency. It is not a gatekeeping exercise.
|
|
|
|
## What Reviewers Should Look For
|
|
|
|
### Always check
|
|
|
|
- Does the change do what the PR description says it does?
|
|
- Are there obvious bugs, edge cases, or error handling gaps?
|
|
- Does it follow [naming conventions](naming-conventions.md)?
|
|
- Are secrets handled correctly per [secrets-and-config.md](secrets-and-config.md)?
|
|
- If AWS resources changed, are Lambda defaults correct per [aws-infrastructure.md](aws-infrastructure.md)?
|
|
|
|
### Watch for
|
|
|
|
- Unused code, dead imports, or leftover debug statements
|
|
- Missing or outdated README updates for functionality changes
|
|
- Hardcoded values that should be in config or Secrets Manager
|
|
- Security concerns (input validation, injection, overly broad IAM policies)
|
|
|
|
### Don't nitpick
|
|
|
|
- Minor formatting differences handled by linters
|
|
- Personal style preferences that don't affect correctness
|
|
- Naming choices that are reasonable even if you'd pick something different
|
|
|
|
## Giving Feedback
|
|
|
|
- Be specific. "This might fail if the list is empty" is useful. "Needs work" is not.
|
|
- Distinguish between must-fix and suggestions. Prefix optional feedback with "nit:" or "suggestion:"
|
|
- Ask questions instead of making assumptions. "Is this intentional?" is better than "This is wrong."
|
|
- If a PR is good, say so. A simple "Looks good" is fine.
|
|
|
|
## Turnaround
|
|
|
|
- Aim to review within one business day of being requested
|
|
- If you can't review in time, say so and suggest another reviewer
|
|
- Don't let PRs sit in review for days without feedback
|
|
|
|
## Approving
|
|
|
|
- Approve when you're confident the change is correct and complete
|
|
- If you left suggestions but the PR is otherwise good, approve with comments rather than blocking
|
|
- One approval is sufficient for most changes
|