Skip to content

Commit 279cb4e

Browse files
derek73claude
andcommitted
AGENTS.md: record four learnings from the 2.0.0rc1 review rounds
- The family-coverage rule: a guard/hint/emitter added to one member of a set belongs on all of it, swept by a parametrized test. This defect shipped three times this cycle (Policy-not-PolicyPatch, decode hint on 3 of 5 entry points, sync roster missing 4 copies). - Anything built on _normalize must also converge -- _title_key drops words that fold away, or 'lt .' stores 'lt ' and goes silently inert. - The scaling test is what guards complexity regressions; calibrate its bound against the weakest quadratic and confirm a plant fails it across repeated runs, not once (a timing test is stochastic). - Release checklist: PRE_RELEASE for pre-release versions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 6a35b49 commit 279cb4e

1 file changed

Lines changed: 5 additions & 1 deletion

File tree

AGENTS.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ uv run sphinx-build -b html docs dist/docs
5050
# and any .rst files that reference config constants or HumanName kwargs
5151
# Also review AGENTS.md for stale commands, architecture notes, or gotchas
5252
# 1. Bump VERSION in nameparser/_version.py (and the `version:` field in CITATION.cff to match)
53+
# For a pre-release, set PRE_RELEASE = 'rc1' (etc.) — __version__ joins it without a dot ('2.0.0rc1'); '' means final
5354
# 2. Stamp "Unreleased" → "X.Y.Z - Month DD, YYYY" in docs/release_log.rst
5455
# 3. git commit + git tag -a vX.Y.Z -m "Release X.Y.Z"
5556
# 4. git push origin master && git push origin vX.Y.Z ← tag must be pushed separately before gh release create
@@ -119,6 +120,7 @@ The 2.0 rewrite lands as underscore-private modules alongside the v1 code. These
119120
- **Canonical field order** — the seven roles in `Role` enum declaration order, defined once and derived everywhere (properties, `as_dict`, reprs, `comparison_key`). Never restate the order literally.
120121
- **Method organization**, fixed section order in every class: fields + `__post_init__` validation → alternative constructors → dunders (construction/equality → protocol → operators) → properties → public methods by concern (access → editing → comparison → rendering delegates) → private helpers last, except a helper serving exactly one section may sit at that section's head. Sanctioned deviation, facade layer only: `HumanName` and the shim `Constants` organize by v1 concern groups (`# -- render defaults --`, `# -- config / parsing --`, `# -- fields --`, ..., dunders and pickle last) — the classes mirror v1's own surface and die in 3.0; the canonical order still binds every core type.
121122
- **Validation is eager and fail-loud**: every `raise` states the offending value, the expected form, and the fix. Exception taxonomy: wrong type — including wrong element type inside a collection, bare `str` where an iterable of strings is expected, or a `Mapping` where a plain iterable is expected — raises `TypeError`; well-typed but unacceptable values raise `ValueError`; failed enum lookups stay `ValueError` for any input (stdlib `EnumType` precedent).
123+
- **Guard, hint, and emit for the WHOLE family, and parametrize the test over it**: a check added to one member of a set belongs on all of it, and the test must sweep the family, not one example. This session shipped `_reject_str_and_mapping` on `Policy` but not `PolicyPatch`, the bytes decode hint on three of five config entry points, and a regex-sync roster missing four of its copies — each a separate follow-up bug that a `{class} × {field} × {bad-value}` parametrization would have caught and a per-example test hid. When you find you're guarding member N, grep for the other members first.
122124
- **Ambiguities are emitted at the DECISION site**: an `Ambiguity` records a fork the parse had to call, not a token that sits in an ambiguous vocabulary. Emit where the branch is taken — the trailing-suffix peel in `_assign`, the delimiter escape's follow-up in `classify` — never by scanning for a `vocab:*-ambiguous` tag. The same tagged token is a genuine fork in one position and unremarkable in another (`do` mid-name in "Joao da Silva do Amaral de Souza" chooses nothing). **A branch that runs but changes nothing is not a decision either** -- the prefix chain's `merge(k, j)` executes even when `j == k + 1`, folding a piece into itself, and keying on "the code got here" reported a fork for all 39 ambiguous particles on "Dr. Van Jr.", where the particle stayed the GIVEN name and `_assign` reported the same token again. Check that the branch actually claimed something (`j > k + 1`) before recording. Structure also structure often settles the question before it arises, which is why `PARTICLE_OR_GIVEN` is deliberately not emitted on the `FAMILY_COMMA` path and `SUFFIX_OR_NAME` is not emitted for "Ma, Jack". The decision site also has the token index and the detail text in hand, which the tag scan would have to reconstruct. **If a fork's two branches are taken in DIFFERENT stages, every one of them needs the emitter** -- `PARTICLE_OR_GIVEN` is decided in `_assign` when the ambiguous particle stays a lone leading piece and in `_group` when a title shifts it off index 0 and the prefix chain claims it, so both report; for two years only the first did. The stage-ownership map in `tests/v2/pipeline/test_state.py` must list `ambiguities` for each such stage, and it passes vacuously until a case row exercises the path, so add the row too. Report BOTH directions of a two-way fork — "John Smith MA" (read as a suffix) and "Jack MA" (read as the family name) are equally guesses. Every kind needs a trigger in `tests/v2/test_contracts.py::_AMBIGUITY_TRIGGERS` (an explicit `None`, strict-xfail, while reserved), and case-table rows pin expected kinds exactly, so a new emitter shows up in both immediately. **Pin the decision, not the vocabulary**: the only titled-particle test used an UNAMBIGUOUS particle, so it walked the right code path and proved nothing about the branch under test -- two criticals passed 1539 tests. A row contrasting the two readings ("John Smith V" against "John Smith B") is what makes an emitter's absence meaningful.
123125
- **A kind is worth adding only if a reader would hesitate too**: the test is not "does the code take a branch" but whether a person reading that input would genuinely be unsure. "Smith, John V" reads as a middle initial to anyone -- the comma settles it -- so reporting it would be noise that teaches callers to ignore the field, which costs more than the missing report. Reachability of the second branch is necessary, not sufficient. Prefer leaving a fork silent and documenting the omission (see the comma paths in concepts.rst) over emitting on input nobody finds ambiguous.
124126
- **Invariants guard harm, not no-ops**: add a constructor check when violating it produces a *wrong parse*, not when it produces *nothing*. A false positive costs a working configuration; a true positive on an inert condition costs the user nothing, so that trade is never worth taking. `suffix_acronyms_ambiguous ∩ suffix_words` is guarded because the overlap loses a family name; `given_name_titles` is not, because an unreachable entry is simply never consulted (see Gotchas). Before adding one, construct the config it forbids and check what actually breaks.
@@ -174,7 +176,9 @@ Add a dedicated `copy.deepcopy()` round-trip test for it too (see `test_regexes_
174176

175177
**`Lexicon.given_name_titles` is deliberately unvalidated against `titles` — do not add a check** — the lookup key is the space-joined run of `Role.TITLE` tokens, built by the parse, and a conjunction inside a run is itself tagged `Role.TITLE`, so `"sir and dame"` is a matchable key whose middle word lives in `conjunctions`. A whole-entry check rejected multi-word entries; a per-word check rejected that one. No static relation over the vocabulary sets decides reachability. An unreachable entry is inert — nothing consults it and nothing misparses — so the condition being "guarded" costs the user nothing while each guard cost a working configuration. If a diagnostic is wanted, it must be non-blocking (a property, a repr note, a warning), never a raise.
176178

177-
**`_normalize` must reach a fixed point** — storage and match-time share the one fold, and `Lexicon.__setstate__` re-validates, so a value that changes on re-normalization changes under its owner. `strip().strip(".")` alone is not idempotent (`'. a .'``' a '``'a'`). The loop is the fix; keep any new stripping inside it.
179+
**`_normalize` must reach a fixed point** — storage and match-time share the one fold, and `Lexicon.__setstate__` re-validates, so a value that changes on re-normalization changes under its owner. `strip().strip(".")` alone is not idempotent (`'. a .'``' a '``'a'`). The loop is the fix; keep any new stripping inside it. **Anything built on `_normalize` must converge too**`_title_key` joins per-word `_normalize` and DROPS words that fold away; keeping the empty slot stored `'lt .'` as `'lt '`, a key match-time can never rebuild (so the entry is silently inert) and `__setstate__` rejects on the next round-trip as "not written by this version".
180+
181+
**Perf regressions are caught by the scaling test, not the absolute-time ones**`tests/v2/test_benchmark.py::test_parse_cost_grows_no_worse_than_linearly` times a repeated unit at n vs 4n over nine shapes (one per pipeline inner loop) and bounds the ratio; the `_thousand_names` tests use constant-size, delimiter-free input and are structurally blind to a complexity regression. Two rules when touching it: calibrate `_MAX_RATIO` against the WEAKEST quadratic's signal (a mixed quadratic surfaces far below the textbook 16×, so the operating point `_BASE` matters more than the bound), and confirm a planted regression fails it across REPEATED runs — one failure is a coin-flip on a timing test. The nine shapes cover different dimensions (segment count only via `commas`, intra-piece accumulation only via `particles`/`conjunctions`); measure before pruning one.
178182

179183
**Expected-failure tests use `@pytest.mark.xfail`** — the conftest parametrized fixture breaks `@unittest.expectedFailure`; always use `@pytest.mark.xfail` instead.
180184

0 commit comments

Comments
 (0)