fix(group): a wholly-credential family-comma segment renders as one entry (#429) - #435
Open
derek73 wants to merge 4 commits into
Open
fix(group): a wholly-credential family-comma segment renders as one entry (#429)#435derek73 wants to merge 4 commits into
derek73 wants to merge 4 commits into
Conversation
…redicates Pure relocation, no behavior change. The predicate reads _is_suffix_piece and _is_leading_title, both of which already live in _group, and #429 needs group to ask it as well as assign. assign imports FROM group, so group is the only place both stages can reach -- the same direction _leading_titles moved in #424, and the layering test names "_group's piece predicates" as the shared home. Its annotations widen to the Sequence/Set spelling its new siblings use; assign passes tuples, group passes lists. _is_leading_title leaves assign's imports with it, having had no other caller there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ntry One comma segment is one suffix entry. group decided which segments those are by INDEX (tail_start: 1 after a suffix comma, 2 after a family comma), which is right wherever assign reads the segment as suffixes for the same structural reason -- but not after a ONE-WORD family comma, where segment 1 is a name slot assign re-reads by CONTENT and routes piece by piece (#296/#325). The roles were right and the render was not: "Smith, MD PhD" suffix 'MD, PhD' -> 'MD PhD' "Smith, CBE MC" suffix 'CBE, MC' -> 'CBE MC' "Smith, PhD Jr." suffix 'PhD, Jr.' -> 'PhD Jr.' -- a comma the writer never typed, while the full-name "John Smith, MD PhD" has rendered 'MD PhD' since 1.4.0. group now asks assign's own predicate over the pieces it just built, so the two stages cannot disagree about what the segment is. Scoped deliberately, and the scoping is the decision: `tail` still gates delimiter-core dropping, which is v1 expand_suffix_delimiter parity (#191) and a TAIL rule. Only the one-entry JOIN follows assign's content read, and the core set is threaded as seg_cores so the widened condition cannot drop cores where `tail` says not to. A run written with commas keeps them ("Smith, MD, PhD"), and a name word still makes it the given-and-suffix walk ("Smith, John Jr."). Three #325 case rows pinned the inserted comma and move with it. The C1 example gains "Smith, MD PhD", which enters the rules corpus; ledger entries for the 2.0.0 and 2.1.0 baselines arrive with it. A fourth written for 1.4.0 EXPLAINED NOTHING and was dropped -- fix(comma-family) lone post-comma piece already claims the name there. Gates: suite green, mypy and ruff clean, differential 0 unexplained at 1.4.0, 2.0.0 and 2.1.0. Left alone, measured: the no-comma path comma-joins a run too ("John Smith MD PhD" renders 'MD, PhD' at 1.4.0, 2.1.0 and master alike). The fix requires FAMILY_COMMA and does not reach it. Closes #429 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #435 +/- ##
=======================================
Coverage 98.59% 98.59%
=======================================
Files 44 44
Lines 3061 3064 +3
=======================================
+ Hits 3018 3021 +3
Misses 43 43 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The review round found the first draft shipped the inverse of the bug it
fixed. It let ANY piece open an entry, as the tail block always had --
safe there, because assign routes every tail piece to SUFFIX, which is
what `tail` means, and wrong off it, where a title piece routes to TITLE.
Two failures, one cause, neither visible to the gates that passed. The
`joined` tag is role-BLIND and the facade heals it for every role:
"Smith, Rev. Dr." title_list ['Rev.','Dr.'] -> ['Rev. Dr.']
"Smith Jr., Mr. Jr." suffix 'Jr., Jr.' -> 'Jr. Jr.'
The second glues a suffix backward across a comma the writer typed --
exactly what #429 exists to stop. The differential compares strings and
cannot see the first; the case table asserts the title STRING, which is
space-joined either way, and could not see it either.
Two joins that had been one, separated: WITHIN a piece the tag renders a
merged piece as one unit whatever role it holds; BETWEEN pieces it
continues an entry, and only a piece rendering into the same run may do
that. Sticky across a piece that is not in the entry, so an interleaved
title does not split its run ("Smith, MD Dr. PhD" -> 'MD PhD'); a
delimiter core still closes it.
Eight case rows and a facade test for the list views, which is the only
surface that shows the title collapse. Both regression guards verified
against a mutation copy -- they fail with the old condition restored.
Prose corrections, all measured by the reviewers:
- The round-trip claim was false AND backwards: str() of a fixed parse
is a no-comma string, which re-parses with the comma back. master was
the str-stable one. Struck from the release log and the case note.
- "one-word family comma" is not the condition -- there is no word-count
gate, so "John Smith, Jr. III" moves too (1.4.0's reading), as does a
title-led "Smith, Dr. MD PhD". Scope restated as it reads.
- The delimiter parity is #206 (021823e, "Apply suffix_delimiter only at
suffix-consumption sites"), NOT #191, the German/Dutch vocabulary PR.
Three code comments carried the error; corrected with it.
- The dormant-rule tell is #373's, and #426 the precedent for dropping a
shadowed rule -- neither #424 entry mentions it.
- "boundary example" in the entry and both ledgers: the example FIRES,
which is why the annotation came off.
- "filed rather than folded in" claimed an issue that does not exist.
C1 gains `_group.py` in `implemented:`, with the verbatim citation the
equality guard requires -- the whole-run half of the rule renders here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ssing /simplify, four angles. Reuse and efficiency returned no changes; the other two found real work. The one that matters: the diff's central conjunct was UNPINNED. Dropping `in_entry and` from the tag test left the whole suite green -- including the guard added for exactly this -- while diverging on 1,152 of 65,725 inputs. That guard pins the sticky entry_open update, not the tag condition. The distinguishing input needs a suffix piece FIRST and then TWO titles, so the second title continues an entry it does not belong to: "Smith, MD Rev. Dr." gives title_list ['Rev.','Dr.'] and the mutant gives ['Rev. Dr.'], with the title STRING identical either way. Added, and verified to fail against the mutation copy. The invariant this change rests on -- one comma segment is one suffix entry -- was stated in NO rule. C1 governs routing; R1 governed word ORDER and never mentioned separation; the only statement was a code comment. That is a large part of why two stages disagreed about it for years, so R1 gains the sentence and two examples, both names already in the corpus (a third, "Smith, MD, PhD", would have moved for a #325 routing reason and been misfiled under #429). Also from the review: - Two case rows removed. family_comma_three_credential_entries is a strict kill-subset of the title-led row AND its note claimed to pin a latch that family_comma_title_between_credentials actually pins -- all three of its pieces are in the entry, so the latch never has to survive a gap. family_comma_title_run_does_not_join kills nothing, by its own note; test_facade.py holds the same input with assertions that can see the collapse. - `family_comma` is redundant by invariant (segment() emits at most one segment for NO_COMMA; SUFFIX_COMMA short-circuits on `tail`). Kept for locality, now documented as unpinnable rather than left to look like missing coverage. - I reintroduced #191 in my own new comment while this same PR corrects it elsewhere. Fixed here and at the two remaining delimiter sites. test_group.py:128 keeps #191 -- it is genuinely about the prefixes. - The predicate moved below _is_suffix_piece, which it calls, restoring the _is_leading_title/_leading_titles pair it had split. - The "whatever role it holds" claim is qualified: no multi-token TITLE piece witnesses that branch in 38,892 generated inputs. - The docstring's regression narration trimmed to the contract sentence plus a pointer; the concrete account lives at the call site. Efficiency recorded rather than optimised: _segment_holds_no_name runs twice per family-comma parse, measured pure recomputation (408 of 506 comma-bearing literals reach both sites, 0 disagreements). Worth 1.2-2.2% of a family-comma parse and 0% of every other. Caching needs a derived ParseState field the sites would not otherwise share, so decisions.md carries the number. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #429.
A space-separated credential run after a family comma rendered with a comma the name never had. The roles were already right — this is #428's remaining half.
The disagreement
One comma segment is one suffix entry.
groupdecided which segments those are by index —tail_startis 1 after a suffix comma, 2 after a family comma — whileassigndecides the same segment by content, with_segment_holds_no_name, and routes it piece by piece (#296 / #325).Those two answers agree everywhere except the segment a family comma leaves behind. There,
groupsaw a name slot andassignsaw a credential run, so the run was routed whole and rendered split.Scope, as the condition actually reads
Any family comma whose following segment holds no name word — not only a one-word family, which is how the first draft described it:
John Smith, Jr. III→ suffixJr. III(also 1.4.0's reading)Smith, Dr. MD PhD→ suffixMD PhD(title-led segments are in)The differential cannot witness the widening: of 1069 corpus names, 303 carry a structure comma, the predicate is true for 36, only 9 of those have ≥2 pieces in the segment, and none combines a multi-word pre-comma name with two suffix pieces. 0 unexplained is evidence about the corpus, not about this class — the case table carries it instead.
The review round found a regression, and it was the inverse of the bug
The first draft let any piece open an entry, as the tail block always had. Safe there —
tailmeans assign routes every piece to SUFFIX. Wrong off it, where a title piece routes to TITLE. Two failures, one cause:The second glues a suffix backward across a comma the writer typed — exactly what this PR exists to stop. The
joinedtag is role-blind and the facade heals it for every role, which is why a suffix-motivated tag corruptedtitle_list.Neither was visible to any gate that passed. The differential compares strings and cannot see a list-view collapse; the case table asserts the title string, which is space-joined either way.
The rule that replaces it separates two joins that had been one:
Sticky across a piece that is not in the entry, so an interleaved title does not split its run (
Smith, MD Dr. PhD→MD PhD). A delimiter core still closes it.Verification
uv run pytestuv run mypy/ruffBoth regression guards were checked against a mutation copy with the old condition restored — they fail, so they are not inert.
test_the_joined_tag_never_reaches_a_titleasserts the list views, the only surface that shows the title collapse.Eight case rows added for classes the mutation matrix found unpinned: title-led runs (384 inputs, previously zero coverage), interleaved titles, three-entry runs, the
seg_idx == 1conjunct, the written-comma negative control, and the full-name twin.Prose corrections from review
All measured, all in the diff:
str()of a fixed parse is a no-comma string, which re-parses with the comma back; master was the str-stable one. Struck.#191was the wrong provenance for the delimiter parity — that is the German/Dutch vocabulary PR. It is Apply suffix_delimiter only at suffix-consumption sites #206 (021823e). Three existing code comments carried the error and are corrected.John van der Berg Vreads familyvan der Berg V— the particle chain and the maiden walk take a trailing numeral as a name word #424 entry mentions it.C1 gains
_group.pyinimplemented:, with the verbatim citation the equality guard requires.Out of scope, measured
The no-comma path comma-joins a run too —
John Smith MD PhDrenders'MD, PhD'at 1.4.0, 2.0.0, 2.1.0 and master alike. The fix requiresFAMILY_COMMAand does not reach it. Recorded indecisions.mdand not yet filed; it is load-bearing for the round-trip story above.🤖 Generated with Claude Code