39 lines
1.7 KiB
Markdown
39 lines
1.7 KiB
Markdown
---
|
|
description: Review a diff or merge request for real bugs and missed requirements
|
|
---
|
|
|
|
Review the change for real problems. Do not edit anything.
|
|
|
|
Target: $ARGUMENTS
|
|
|
|
When the target names a merge request or pull request, review that change. When it names a
|
|
file or directory, review what is uncommitted there. When it is empty, review the current
|
|
working diff.
|
|
|
|
How to review:
|
|
|
|
1. Read the change, then read the code around it. Context you did not read is context you
|
|
will misjudge.
|
|
2. Look for real problems only: logic that is wrong, a case that was missed, a contract
|
|
broken, data lost, an error swallowed, a race introduced, something the change claims to
|
|
do that it does not do.
|
|
3. Check the change against whatever guidance governs the code it touches. Guidance is
|
|
guidance for writing code, so not all of it applies to a review.
|
|
4. Read the git history of the lines being modified. A change that looks fine in isolation
|
|
often breaks a reason the code is the way it is.
|
|
5. Leave these out: style, naming, formatting, import order, missing tests, missing
|
|
documentation, and anything a linter, typechecker, or compiler will catch. CI runs those.
|
|
Also leave out problems on lines the change did not touch.
|
|
|
|
Output:
|
|
|
|
- Order findings by severity, highest first.
|
|
- Each finding gets the file and line, what is wrong, and why it matters. Link to code
|
|
rather than quoting long stretches of it.
|
|
- Say which requirement or guideline is broken when that is the reason.
|
|
- When a finding is a judgement call rather than a definite bug, say so plainly instead of
|
|
dressing it up as certain.
|
|
- If there is nothing worth reporting, say that and stop. Do not invent findings to fill
|
|
the space.
|
|
|