Skip to content

Commit 3ce8314

Browse files
derek73claude
andcommitted
Make the no-mutation leak tests self-maintaining; document the invariant
Replace the hand-enumerated _WATCHED_ATTRS list with a structural snapshot derived from Constants.__getstate__(), the canonical listing of an instance's own config. A collection added to Constants later is watched automatically — there is no list to remember to update (unlike conftest's _COLLECTION_CONFIG_ATTRS, which remains the only manual one). A sanity assert fails loud if the discovery ever stops covering the five historically-mutated sets, and the snapshot now also covers TupleManager collections and scalar overrides. Update AGENTS.md: fix the two Architecture lines that still described parse_pieces()/join_on_conjunctions() as writing into the constants, and add a gotcha documenting the parsing-never-writes-config invariant and the _derived_* overlay recipe for future derived categories. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 268b192 commit 3ce8314

2 files changed

Lines changed: 45 additions & 15 deletions

File tree

AGENTS.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,9 +94,9 @@ Each module defines a plain Python set of known name pieces:
9494
Parse flow:
9595
1. `pre_process()` — strips nicknames/maiden names (parenthesis/quotes, routed to `nickname_list`/`maiden_list` per `Constants.nickname_delimiters`/`maiden_delimiters`) and emoji, fixes "Ph.D." variant spellings
9696
2. Split on commas → 1 part (no comma), 2 parts (suffix-comma or lastname-comma), 3+ parts
97-
3. `parse_pieces()` — splits on spaces, detects dotted abbreviations like "Lt.Gov." and adds them to constants dynamically
97+
3. `parse_pieces()` — splits on spaces, detects dotted abbreviations like "Lt.Gov." and registers them as per-parse derived titles/suffixes (the `_derived_*` sets — never the config; see Gotchas)
9898
4. `join_on_conjunctions()` — merges pieces adjacent to conjunctions into single tokens (e.g. `['Secretary', 'of', 'State']``['Secretary of State']`); also joins prefix particles to the following lastname token
99-
— a piece merged with a title *or prefix* neighbor is re-registered into that constant set so later steps still recognize it, e.g. `von`+`und`+`zu` → registered as a prefix so the whole phrase joins to the last name (German "von und zu"; PR #191)
99+
— a piece merged with a title *or prefix* neighbor is re-registered into the per-parse derived title/prefix set so later steps still recognize it, e.g. `von`+`und`+`zu` → registered as a derived prefix so the whole phrase joins to the last name (German "von und zu"; PR #191)
100100
4a. `_join_bound_first_name()` — called immediately after step 4 in all three paths that build a first-name-bearing token sequence: no-comma, lastname-comma (post-comma pieces), and suffix-comma (`parts[0]`); merges bound given-name prefixes (e.g. "abdul") with the next piece before the assignment loop runs; suffixes are still in `pieces` at this point, so the `reserve_last` guard must count non-suffix pieces only. Not called for the lastname portion itself (`lastname_pieces` in the lastname-comma path) — that token sequence is a surname, not first-name text, so the join must not apply there. The join lives at each of these three call sites individually rather than inside `parse_pieces()` itself. That's because `parse_pieces()` is also called for the lastname portion with the same `additional_parts_count` value used for the (joinable) post-comma given-names portion, so `additional_parts_count` alone can't disambiguate which callers want the join.
101101
5. Iterates pieces, assigning to `title_list`, `first_list`, `middle_list`, `last_list`, `suffix_list`
102102
6. `post_process()``handle_firstnames()` swaps first/last when only a title + one name; then, gated on `patronymic_name_order`, `handle_east_slavic_patronymic_name_order()` and `handle_turkic_patronymic_name_order()` reorder Russian-formal-order and reversed Turkic patronymics; then, gated on `middle_name_as_last`, `handle_middle_name_as_last()` folds `middle_list` into `last_list`; finally `handle_capitalization()` applies optional auto-cap. Any new `self._attr` used by `post_process()` helpers must be initialized in `__init__` (with its default value) — the direct-kwargs path bypasses `parse_full_name()`, so the attribute won't exist otherwise.
@@ -112,7 +112,7 @@ Each named attribute (`title`, `first`, etc.) is a `@property` that joins its co
112112
4. conftest auto-restores scalar CONSTANTS between tests, but tests that *set* CONSTANTS mid-run still need their own try/finally
113113
5. Update `docs/customize.rst`'s constants list (and `docs/usage.rst` if it affects a documented example) — don't wait for the release checklist, these `.rst` docs aren't covered by CI so a stale one can ship silently. Check `AGENTS.md` itself too (Extension Patterns, Gotchas, the Architecture section) for now-stale attribute/test names or descriptions — it has the same blind spot as the `.rst` docs and the release checklist only catches it at release time.
114114

115-
**Adding a new mutable/collection `Constants` attribute** (a `SetManager`/`TupleManager`-backed group, e.g. `nickname_delimiters`/`maiden_delimiters`): add it to `_COLLECTION_CONFIG_ATTRS` in `tests/conftest.py`, or tests that mutate the global `CONSTANTS` copy will leak state into later tests. Contents must be deep-copyable (the snapshot uses `copy.deepcopy`) — already true for the existing manager types.
115+
**Adding a new mutable/collection `Constants` attribute** (a `SetManager`/`TupleManager`-backed group, e.g. `nickname_delimiters`/`maiden_delimiters`): add it to `_COLLECTION_CONFIG_ATTRS` in `tests/conftest.py`, or tests that mutate the global `CONSTANTS` copy will leak state into later tests. Contents must be deep-copyable (the snapshot uses `copy.deepcopy`) — already true for the existing manager types. (The parse-no-mutation tests in `tests/test_constants.py` need no such registration — they discover collections structurally via `Constants.__getstate__()`; conftest's list is the only manual one.)
116116

117117
Add a dedicated `copy.deepcopy()` round-trip test for it too (see `test_regexes_deepcopy_roundtrip`/`test_nickname_delimiters_deepcopy_roundtrip` in `tests/test_constants.py`), not just reliance on conftest's autouse snapshot/restore exercising it incidentally. `TupleManager`/`RegexTupleManager.__getattr__` answer *any* unknown attribute lookup — including dunder probes like `__deepcopy__` — so a new manager subtype or a `__getattr__` tweak can silently break `copy.deepcopy` (this bit `RegexTupleManager` before the dunder-lookup guard was added). A direct test on the new attribute's own manager instance catches that where the conftest fixture, which never asserts on the copy, would not. As with the scalar-attribute pattern above, also check `AGENTS.md` itself for now-stale references when you touch this.
118118

@@ -128,6 +128,8 @@ Add a dedicated `copy.deepcopy()` round-trip test for it too (see `test_regexes_
128128

129129
## Gotchas
130130

131+
**Parsing must never write into `Constants`** — parse-time derived recognition (dotted abbreviations like "Lt.Gov.", conjunction-joined titles/prefixes like "Mr. and Mrs." / "von und zu") lives in per-instance `HumanName._derived_titles` / `_derived_suffixes` / `_derived_conjunctions` / `_derived_prefixes` sets, consulted by `is_title`/`is_suffix`/`is_conjunction`/`is_prefix`/`is_rootname`, reset at the start of `parse_full_name()`, and backfilled in `__setstate__` for pre-existing pickles. Never `self.C.<set>.add()` during parsing — `self.C` is usually the shared `CONSTANTS` singleton, so a write makes parse results order-dependent and thread-unsafe (this was a real bug through 1.2.x). `ParsingDoesNotMutateConfigTests` (`tests/test_constants.py`) enforces the invariant by snapshotting the whole config around a parse; it discovers collections structurally via `Constants.__getstate__()`, so new `Constants` collections are watched automatically — nothing to register. If a new derived category is ever needed: add a `_derived_*` set in `__init__`, reset it in `parse_full_name()`, backfill it in `__setstate__`, consult it in the matching `is_*` predicate (store `lc()`-normalized values, mirroring `SetManager`), and add a leak test with a name that triggers it.
132+
131133
**Titles permanently shadow first names — be conservative** — any word in `TITLES` is always consumed as a title and can never be parsed as a first name. `"Dean"` is the canonical example: it's a common academic title *and* a common given name, so it is intentionally absent from the default titles (see `docs/customize.rst` — users who need it add it via opt-in `Constants`). Before adding a word to `TITLES`, ask: "Could this plausibly be someone's given name in any culture?" If yes, don't add it globally; it belongs in caller-supplied `Constants` instead. This same caution applies to international honorifics — `Prince`, `Sheikh`, `Frau` are all first names in some contexts. It also applies to any prefix sub-set gated on "never a first name": obscure-looking foreign particles are surprisingly often real given names — `Von` (Von Miller), `Vander` (Brazilian, also the Arcane character). When unsure, exclude — a missing member just means that name isn't auto-handled, whereas a wrong member misparses a real person.
132134

133135
**`is_leading_title()` infers titles beyond the `TITLES` set, but only in leading position** — an unrecognized multi-letter word ending in a single trailing period (matched via the `period_abbreviation` regex, `{2,}` letters) is treated as a title when it appears before the first name is set, e.g. `"Major. Dona Smith"``title='Major.'`. It's distinct from `is_title()` and does not mutate `C.titles`, so the periodless form (`"Major"`) is unaffected elsewhere. The `{2,}` length requirement — not a separate initials check — is what excludes single-letter initials like `"J."` from being swallowed as titles; the same word after the first name is left as a middle name. (#109; see `docs/usage.rst` "Leading Period-Abbreviation Titles")

tests/test_constants.py

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -499,17 +499,48 @@ class ParsingDoesNotMutateConfigTests(HumanNameTestBase):
499499
on input order, and concurrent parsing raced on the shared sets.
500500
"""
501501

502-
_WATCHED_ATTRS = ('titles', 'prefixes', 'conjunctions',
503-
'suffix_acronyms', 'suffix_not_acronyms')
502+
@staticmethod
503+
def _config_snapshot(constants: Constants) -> dict:
504+
"""Snapshot every piece of configuration ``constants`` owns.
505+
506+
Enumerated via ``Constants.__getstate__()`` — the canonical listing of
507+
an instance's own config (descriptor-backed names mapped to their
508+
public form, private caches like ``_pst`` excluded) — so a collection
509+
added to ``Constants`` later is watched automatically, with no
510+
attribute list to keep in sync.
511+
"""
512+
snap = {}
513+
for attr, value in constants.__getstate__().items():
514+
if isinstance(value, SetManager):
515+
snap[attr] = set(value)
516+
elif isinstance(value, TupleManager):
517+
snap[attr] = dict(value)
518+
else:
519+
snap[attr] = value # scalar override
520+
# Fail loud if the structural discovery ever stops seeing the sets
521+
# the parser historically leaked into.
522+
assert {'titles', 'prefixes', 'conjunctions',
523+
'suffix_acronyms', 'suffix_not_acronyms'} <= set(snap), \
524+
"config snapshot no longer covers the historically-mutated sets"
525+
return snap
526+
527+
def _assert_config_unchanged(self, constants: Constants, before: dict, parsed: str) -> None:
528+
after = self._config_snapshot(constants)
529+
diffs = []
530+
for attr in sorted(set(before) | set(after)):
531+
b, a = before.get(attr), after.get(attr)
532+
if b != a:
533+
if isinstance(b, set) and isinstance(a, set):
534+
diffs.append(f"{attr}: added {sorted(a - b)}, removed {sorted(b - a)}")
535+
else:
536+
diffs.append(f"{attr}: {b!r} -> {a!r}")
537+
self.assertEqual(diffs, [], f"parsing {parsed!r} changed the config: {diffs}")
504538

505539
def _assert_parse_leaves_config_unchanged(self, name: str) -> HumanName:
506540
from nameparser.config import CONSTANTS
507-
before = {a: set(getattr(CONSTANTS, a)) for a in self._WATCHED_ATTRS}
541+
before = self._config_snapshot(CONSTANTS)
508542
hn = HumanName(name)
509-
for attr, snapshot in before.items():
510-
leaked = set(getattr(CONSTANTS, attr)) - snapshot
511-
self.assertEqual(leaked, set(),
512-
f"parsing {name!r} leaked {leaked!r} into CONSTANTS.{attr}")
543+
self._assert_config_unchanged(CONSTANTS, before, name)
513544
return hn
514545

515546
def test_period_joined_title_does_not_leak_into_titles(self) -> None:
@@ -542,12 +573,9 @@ def test_prefix_conjunction_join_does_not_leak_into_prefixes(self) -> None:
542573

543574
def test_instance_owned_constants_not_mutated_by_parsing(self) -> None:
544575
hn = HumanName("", constants=None)
545-
before = {a: set(getattr(hn.C, a)) for a in self._WATCHED_ATTRS}
576+
before = self._config_snapshot(hn.C)
546577
hn.full_name = "Lt.Gov. John Doe"
547-
for attr, snapshot in before.items():
548-
leaked = set(getattr(hn.C, attr)) - snapshot
549-
self.assertEqual(leaked, set(),
550-
f"parsing leaked {leaked!r} into the instance's own C.{attr}")
578+
self._assert_config_unchanged(hn.C, before, "Lt.Gov. John Doe")
551579
self.m(hn.title, "Lt.Gov.", hn)
552580

553581
def test_derivations_reset_between_parses_of_same_instance(self) -> None:

0 commit comments

Comments
 (0)