Skip to content

Commit bacb42b

Browse files
derek73claude
andcommitted
Apply type-design review fixes to the pipeline state contract
The ParseState ownership map said roles belong to assign/post_rules exclusively, but group writes maiden roles -- an ownership contract wrong in one place teaches readers to distrust it everywhere. Fixed the map, noted post-group segments staleness and the sole-producer text invariant on WorkToken, documented PendingAmbiguity's dangling- index tolerance and Parser's None-default resolution, made assemble's role fallback say what it means, and pinned the whole ownership map mechanically: a new test folds the case corpus stage by stage and asserts each stage changes only the fields it owns. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 9347848 commit bacb42b

4 files changed

Lines changed: 50 additions & 5 deletions

File tree

nameparser/_parser.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,10 @@
2626
class Parser:
2727
"""Immutable, thread-safe, picklable by construction (spec §4): all
2828
validity checking happens at construction; a Parser that constructs
29-
successfully cannot fail at parse time on any str content."""
29+
successfully cannot fail at parse time on any str content. The None
30+
field defaults resolve in __post_init__; after construction both
31+
fields are always non-None (the annotations state the steady-state
32+
truth, hence the assignment ignores on the defaults)."""
3033

3134
lexicon: Lexicon = None # type: ignore[assignment] # None -> default()
3235
policy: Policy = None # type: ignore[assignment] # None -> Policy()

nameparser/_pipeline/_assemble.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ def assemble(state: ParseState) -> ParsedName:
2323
for i, t in enumerate(state.tokens):
2424
if i in dropped:
2525
continue
26-
final[i] = Token(t.text, t.span, t.role or Role.GIVEN, t.tags)
26+
role = t.role if t.role is not None else Role.GIVEN
27+
final[i] = Token(t.text, t.span, role, t.tags)
2728
ambiguities = tuple(
2829
Ambiguity(p.kind, p.detail,
2930
tuple(final[i] for i in p.indices if i in final))

nameparser/_pipeline/_state.py

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,9 @@
2121
@dataclass(frozen=True, slots=True)
2222
class WorkToken:
2323
"""One tokenized word. role stays None until assign; extracted
24-
nickname/maiden tokens arrive with their role pre-set."""
24+
nickname/maiden tokens arrive with their role pre-set. text is
25+
always the exact original slice (tokenize is the sole producer;
26+
the anti-#100 invariant depends on it)."""
2527

2628
text: str
2729
span: Span
@@ -53,8 +55,11 @@ class ParseState:
5355
dataclasses.replace. Fields are filled progressively:
5456
extract_delimited -> extracted/masked; tokenize -> tokens (span-
5557
sorted)/comma_offsets; segment -> segments/structure; classify ->
56-
token tags; group -> pieces/dropped; assign/post_rules -> token
57-
roles; ambiguities accumulate anywhere."""
58+
token tags; group -> pieces/piece_tags/dropped AND maiden token
59+
roles; assign/post_rules -> the remaining token roles; ambiguities
60+
accumulate anywhere. Post-group, segments may retain indices of
61+
dropped tokens -- assign iterates pieces, never segments. This
62+
ownership map is pinned by tests/v2/pipeline/test_state.py."""
5863

5964
original: str
6065
lexicon: Lexicon

tests/v2/pipeline/test_state.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,3 +29,39 @@ def test_state_is_frozen_and_replace_works() -> None:
2929
def test_worktoken_carries_optional_role() -> None:
3030
t = WorkToken("Jack", Span(6, 10), role=Role.NICKNAME)
3131
assert t.role is Role.NICKNAME
32+
33+
34+
def test_stage_field_ownership() -> None:
35+
# The ParseState docstring's ownership map, pinned mechanically: run
36+
# the whole case corpus through the fold stage by stage and assert
37+
# each stage only changes the fields it owns. Converts the prose
38+
# contract into a test (a future stage clobbering another stage's
39+
# field fails here, not in a distant assertion).
40+
import dataclasses as _dc
41+
42+
from nameparser import Lexicon as _Lexicon
43+
from nameparser._pipeline import STAGES
44+
45+
from ..cases import CASES
46+
47+
ownership = {
48+
"extract_delimited": {"extracted", "masked", "ambiguities"},
49+
"tokenize": {"tokens", "comma_offsets"},
50+
"segment": {"segments", "structure", "ambiguities"},
51+
"classify": {"tokens"},
52+
"group": {"tokens", "pieces", "piece_tags", "dropped"},
53+
"assign": {"tokens", "ambiguities"},
54+
"post_rules": {"tokens"},
55+
}
56+
assert {s.__name__ for s in STAGES} == set(ownership)
57+
for case in CASES:
58+
state = ParseState(original=case.text, lexicon=_Lexicon.default(),
59+
policy=case.policy or Policy())
60+
for stage in STAGES:
61+
before = {f.name: getattr(state, f.name)
62+
for f in _dc.fields(state)}
63+
state = stage(state)
64+
changed = {name for name, value in before.items()
65+
if getattr(state, name) != value}
66+
assert changed <= ownership[stage.__name__], (
67+
f"{case.id}: {stage.__name__} changed {changed - ownership[stage.__name__]}")

0 commit comments

Comments
 (0)