mirror of
https://github.com/domainaware/parsedmarc.git
synced 2026-08-01 13:12:18 +00:00
PR #851 was the first cycle run with all prior review guidance loaded in context. The author's own seam-checklist pass caught three real defects before the PR opened, but a fresh-context reviewer still caught four more across two rounds. Three new rules drawn from the shapes of those misses: - A fix made during review is new code with zero review coverage; touching one direction of a paired protocol obligates re-deriving the inverse direction, including version-skew inputs. - Verification means CI's literal commands from the repo root, not a plausible subset; fix noise via config exclusions, don't narrow the command. - Count enumerations in prose against the code-defined set they enumerate. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
4e80047e68
commit
2084e5d162
@@ -197,6 +197,12 @@ Three more rules, from the PR #849 review cycle (a fresh automated reviewer caug
|
||||
- **An extracted helper is new API surface.** Code promoted out of a call site for reuse inherits none of that call site's implicit guarantees. The original inline code could assume its inputs were already clamped; the reusable helper cannot — it needs its own input validation (raised eagerly, not deferred until a generator is first iterated) and its own docstring↔behavior check, even when every *current* caller happens to be safe. On #849 the promoted pool helper was missing an `n_procs` bound check its old call site had made unnecessary, and its docstring described a stop path that didn't match what the implementation actually did.
|
||||
- **End with a fresh-context review, not a self re-read.** The author's "cold re-read" is never cold — it confirms the model of the code the author already holds, which is exactly the blindness a fresh reader doesn't share (see the #839 lesson above; #849 reproduced it two rounds running). Before opening a PR, run a review pass whose reviewer has seen *only* the final diff — no plan, no conversation history, no memory of writing it (e.g. a subagent given just the diff, or an external reviewer). Findings from that pass are triaged like any external review: fix what's real, push back with cited reasoning on what isn't.
|
||||
|
||||
Three more rules, from the PR #851 review cycle (the first run with all of the above loaded in context — the author's own seam-checklist pass did catch three real defects before the PR opened, so the habits fire, but a fresh-context reviewer still caught four more):
|
||||
|
||||
- **A fix made during review is new code with zero review coverage.** #851's review pass fixed the fragile hand-enumerated field list in `ParserConfig.__getstate__` — and stopped there. The symmetric obligation that fix implied in `__setstate__` went unexamined (state pickled by an older version lacks later-added fields, and `__init__` never runs during unpickling, so those fields end up not defaulted but *unset*), and the external reviewer caught it. Touching one direction of a paired protocol (`__getstate__`↔`__setstate__`, save↔load, encode↔decode) obligates re-deriving the inverse direction — including version-skew inputs (old data into new code) that no current fixture produces. The seam checks apply to your own review fixes; the review isn't done when the fixes are written.
|
||||
- **Verification means CI's literal commands from the repo root, not a plausible subset.** #851's local checks scoped to `parsedmarc/ tests/` declared the tree green while CI's `ruff format --check .` failed on a Python example inside `docs/source/usage.md` — ruff formats code blocks in Markdown, and `docs/` was outside the scoped run. Read the workflow file and run the same commands with the same scope before opening a PR. When repo-wide runs are noisy because of untracked local directories, fix the exclusion in config rather than narrowing the command — a narrowed command is a different check that happens to share a name.
|
||||
- **Count enumerations against the set they enumerate.** The new library-usage docs listed "the main entry points" and the next paragraph asserted that "each of these functions" accepts `config=` — but the list named seven of the eight `config=`-accepting functions. Whenever prose enumerates a code-defined set (functions gaining a kwarg, supported outputs, config keys), derive the set from the code (grep the signatures) and count both sides; a reader can't tell an intentional subset from an omission.
|
||||
|
||||
## Releases
|
||||
|
||||
A release isn't done until built artifacts are attached to the GitHub release page. Full sequence:
|
||||
|
||||
Reference in New Issue
Block a user