fix(#459): repair one-case suffix acronyms to all-caps in case repair - #521
Conversation
Gated on the SUFFIX role; exceptions map consulted first so md/phd keep their special casing. R4 example added; corpus regenerated.
derek73
left a comment
There was a problem hiding this comment.
Thanks for this -- it's a well-formed fix and the design is accepted as-is: all-caps repair for a word the parse put in the suffix role from the acronym vocabulary, exceptions map consulted first, placed before the Mac/Mc rule (which matters: mcse would otherwise come out McSe). The role gate is the right instrument and the right width; anh van do staying Do is exactly the control we want. The wider design in #459 (letter masks, md leaving the map, the given-role half of QC MP) stays on #459, which I'll rescope rather than close, so please read the deferrals below as scoping, not rejection.
Measured on the PR head over the differential corpora (1143 names): 5 move on the default capitalized() path, 71 under force=True; 122 names carry acronym vocabulary in a suffix token and 11 of those are single-cased, so the honest reach is 5 of 11 eligible.
Blocking (two behavior items)
-
CI fails on the Sphinx doctest:
docs/customize.rstaround line 166 expectsstr(parse("jane smith dds").capitalized())to be'Jane Smith Dds'. That passage demonstratescapitalization_exceptionson a word your fix now handles by default, so the example loses its contrast even once the string is corrected. Please swap the demo word for one that is not insuffix_acronyms--dphilis unshipped today and is the very word #459 names as the caller-added case, so the doc and the deferred design point at the same example. While there, the parenthetical two lines above ("phd"->"PhD") has been wrong for a while: the shipped map givesPh.D.. -
phneeds to leavesuffix_acronymsin this PR. The fragment exists only to prop up the spacedPh. D., and with your clause it now reads all-caps on the default path, not only under force:john smith ph. d.->John Smith PH. D.,smith, ph. d.->Smith PH. D.. Measured withphremoved: 0 corpus names move on any role field, 0 move undercapitalized(), and the 13PH. D.force movers go back toPh. D.;john smith phdandph.d.still givePh.D.. The 2.3.0 release log already carries three bullets of this shape (rai,cha,esqleaving the acronym set underdecisions.md#suffix-acronym-collisions), so it's a one-line addition there, not new scope.
Blocking (house conventions -- see AGENTS.md's same-PR amendment rule)
-
rules.md#R4's statement doesn't yet say what your example line illustrates: the rule text covers the casing a vocabulary entry records and the Mac/Mc convention, and an all-caps repair keyed on the suffix role is a new conjunct. One sentence in the statement, plus a boundary example line"john smith jr" -> capitalized="John Smith Jr"(the boundary #459 itself states; the rules-doc runner executes it on the core path, which also covers the "is there a core-path test?" question). -
A
docs/design/decisions.mdbullet under theR4entry: the decision and its narrow scope, the reach with population first (5 of 11 eligible, 122 in the population, 71 forced), a recompute recipe that names the comparator (the pre-change_cap_wordfromgit show d37b8ec:nameparser/_render.py, swapped in-process), and the accepted costs -- see item 6. -
A
docs/release_log.rst2.3.0 Behavior Changes bullet in the house style: bold lead, before/after (HumanName("JOHN SMITH MBA").capitalize()givesJohn Smith MBAwhere every release since 1.4.0 gaveJohn Smith Mba;jd->JD;QC MP->Qc MPwith the given-name half unchanged;md/phdunchanged;jruntouched), provenance by entry name, and(#459)rather thancloses #459.
Corrections to prose, non-blocking but they should land
- The comment says every other
suffix_acronymsentry "is all-caps by definition" and that the map's five "need non-all-caps spelling".bscandmscare conventionally mixed-case (Dr. med. univ. Margit Popp, MScis a corpus name and now readsMSCunder force), andii/iii/ivneed all-caps and aresuffix_words, not acronyms. The clause is still the right default for an initialism; the justification just needs to say so and name the mixed-case cost as deferred. Likewise the PR description'sed -> EDargument: the role gate doesn't prevent it, it moves which parse triggers it --john smith ed->John Smith ED,john smith ba->BA,smith, ms.->MS.on the default path, because the ambiguous five (ba,do,ed,jd,ma) are insuffix_acronyms. That's an accepted cost of the #342/#454 class #459 already accepts, and the alternative (reading classify'svocab:suffixtag) was measured and costsjd->Jd, so your choice is the right one; the decisions bullet should record the fork with those numbers. Also a stray extra blank line in the class body oftests/test_capitalization.py.
Deferred to the rescoped #459, so nobody relitigates them here: letter masks (BSC/MSC -> BSc/MSc), md leaving the map, QC in the given role, and the within-run inconsistency where vocabulary coverage shows (John Doe CLU, CFP, Lutc).
…ules/decisions/release-log, correct the comment - docs/customize.rst: dds->dphil demo (dds now handled by default); phd parenthetical -> Ph.D. - config/suffixes.py: ph left suffix_acronyms (read all-caps on the default path; 13 force movers back to Ph. D.) - rules.md R4: all-caps clause sentence + john smith jr boundary line; regenerated corpus_rules.jsonl - decisions.md R4: derek73#459 bullet (scope, reach 5/11-71, recompute recipe, accepted costs) - release_log.rst 2.3.0: behavior bullet - _render.py comment: bsc/msc mixed-case + ii/iii/iv are suffix_words; test blank line
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #521 +/- ##
=======================================
Coverage 98.78% 98.79%
=======================================
Files 45 45
Lines 3636 3638 +2
=======================================
+ Hits 3592 3594 +2
Misses 44 44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks -- this addresses everything I asked for, and the fix itself is unchanged from what I approved. What's left is docs-only, plus a rebase, because 2.3.0 shipped on the 12th and master moved 40 commits under you. I re-ran the suite and the differential gate on your two commits rebased onto current master: 7978 passed, and the gate exits 0 at all five baselines with 0 unexplained diffs, so the If you'd rather not do a third round on house conventions, say so and I'll push these to your branch myself (as a merge from master plus one docs commit, no force-push, and I'll leave a comment saying what landed). Either way is fine with me. Mechanical
Corrections to the decisions.md R4 entry
Corrections to the rules.md R4 statement
Also refresh the PR description's measurement paragraph and test count once you've done 3. Everything else re-measured and held on the rebased tree: the |
|
I'll take you up on that - stopping at this round. Please push the master merge and the docs commits onto my branch; I'll re-run the suite on my end and watch CI. Thanks for carrying the heavy part. |
…nym-caps # Conflicts: # docs/design/decisions.md # docs/design/rules.md # tools/differential/corpus_rules.jsonl
…nd the ph removal is on the record Maintainer fix-up on PR derek73#521 after the merge from master, the eight items of the 2026-09-16 review comment: - decisions.md R4: the reach is remeasured on the merged tree with the recipe's comparator named (master's `_cap_word` at 23e52dc), and the two earlier snapshots are explained rather than replaced -- the `ph` removal took 13 forced movers back to `Ph. D.`, and the rows PRs derek73#530/derek73#532/derek73#534 added account for the rest of the drift. The exceptions map's job is stated as the spellings `str.capitalize()` gets wrong, not "non-all-caps" (II/III/IV are all-caps); the precedence over Mac/Mc and the role-not-tag reach of the gate are recorded. - decisions.md: an `Excluded (SUFFIX_ACRONYMS — ph)` block beside esq's, so a wordlist sweep does not put the fragment back. - rules.md R4: the non-acronym clause is qualified by the exceptions map (`john smith ii` -> `II`), the acronym repair's precedence over the Mac/Mc convention is stated and pinned by a `john smith mcse` row (`McSe` without it), and the Accepted paragraph names which clause it is about and states the acronym repair's contrast on a spliced suffix. - release_log.rst: the bullet moves from the shipped 2.3.0 section to 2.4.0 with the current digits, and `ph` leaving the set gets its own bullet in the rai/cha shape. - corpus_rules.jsonl regenerated (three new R4 rows). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…h. is pinned and classified The docs review of the fix-up found what the removal's measurement could not: `ph` also carried a bare trailing `Ph.` -- no `D.` behind it -- as a credential on every release since 1.4.0, and with the entry gone `John Smith Ph.` reads middle Smith, family Ph. The corpus had no name of that shape, so "0 role movers" was true and blind, the case AGENTS.md warns of. - tests/v2/cases.py: a shape-1 row carries `John Smith Ph.` at the positional reading, classified fix(derek73#459), beside the esq rows the same criterion decided. - corpus_shapes.jsonl regenerated with it. - all five ledgers: a `change(suffix-acronym-collisions) ph leaves the acronym set` rule, literal and exactly as wide as the diff; the gate explains that one name at every baseline with nothing unexplained. - test_ledger_guards.py: the rule's corpus claim recorded per ledger, and its negative controls (`Ph. D.` in both positions, `phd`, `Ph.D.`). - decisions.md `Excluded (SUFFIX_ACRONYMS — ph)` and the 2.4.0 release-log bullet state the cost, the classification, and the add-back for a caller who wants the old reading. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Pushed three commits onto your branch, no force-push -- your two commits are untouched ancestors. What landed and why, keyed to the numbered items above: 76bcc86 -- merge master. Items 1 and 2's rebase half. Master had moved 80 commits; the conflicts were 9eec6b4 -- the docs fix-up. Items 2 through 8:
16b66c6 -- one thing the review of the fix-up found that neither of us had. Verified on the pushed head: 9675 passed / 324 skipped / 4 xfailed; differential gate exit 0 at all five baselines with 0 unexplained; Sphinx doctests pass; the design-docs review ran twice over the docs/design changes and its findings are what 16b66c6 fixes. I appended a short maintainer section to the PR description with the current numbers rather than editing your text. Once CI is green on your side I'll merge. |
What
HumanName('JOHN SMITH MBA').capitalize()returned'John Smith Mba'— a credential acronym title-cased as an ordinary word (#459).CAPITALIZATION_EXCEPTIONSonly carries the entries that spell differently (md->M.D.,phd->Ph.D., and the generationalii/iii/iv); every othersuffix_acronymsentry fell through tostr.capitalize()and got title-cased (mba->Mba,jd->Jd,qc->Qc,mp->Mp).Fix
In
_cap_word, after the exceptions-map lookup and before the Mac/Mc rule, a word whose period-free form is a knownsuffix_acronymsentry repairs toword.upper().Gated on
role is Role.SUFFIXso a word that is in the vocabulary but parsed as a family name (anh van do->Anh Van Do) still repairs as an ordinary name word, not an acronym. The exceptions map is consulted first, somd->M.D.andphd->Ph.D.are unchanged.Out of scope
The issue's
QC MPexample: the parser assignsQCthe GIVEN role andMPthe SUFFIX role, so onlyMPrepairs to caps here and the given-name half stays title-cased, because the gate is keyed on the SUFFIX role rather than on "this word is an acronym". The gate does NOT, however, prevent the ambiguous five (ba,do,ed,jd,ma, all insuffix_acronyms) from being uppercased when they parse in the SUFFIX role:john smith ed->John Smith ED,john smith ba->BA, andsmith, ms.->MS.on the default path. That is the same #342/#454-class cost the issue already accepts, and the alternative (reading classify'svocab:suffixtag instead of the role) was measured and costsjd->Jd, so the role gate is the right instrument. The letter-mask design (phd->PhD,bsc->BSc),mdleaving the map, and the given-role half ofQC MPstay on the rescoped #459.Docs & tests
tests/test_capitalization.py(all-caps suffix, exceptions still win, family-name-in-vocab stays title-case).rules.mdR4: added ajohn smith mbaexample and thejohn smith jrboundary; regeneratedtools/differential/corpus_rules.jsonl.docs/customize.rst: thecapitalization_exceptionsdemo word swapped fromdds(now handled by default) todphil(unshipped, the caller-added case Case repair: letter masks for the exceptions map (phd → PhD, bsc → BSc), md leaves the map, and roman numerals past iv (john smith xi → Xi) #459 names); thephd->PhDparenthetical corrected toPh.D..phleftsuffix_acronyms(it existed only to prop up the spacedPh. D., which the new clause would read all-caps on the default path); the 13PH. D.force movers go back toPh. D., andphd/ph.d.still givePh.D..docs/design/decisions.mdR4 entry anddocs/release_log.rst2.3.0 bullet.Full suite: 7610 passed, 179 skipped, 4 xfailed.
Maintainer fix-up (2026-09-22, derek73)
Three commits pushed onto the branch after the contributor handed it over (76bcc86 merge from master, 9eec6b4 docs fix-up, 16b66c6 the
phparity-break record); the two contributor commits are unchanged ancestors. Current measurements supersede the paragraph above: on the merged tree, 1338 distinct corpus names, 178 carry an acronym suffix token, 30 single-case, 22 move on the defaultcapitalize()path and 119 underforce=True; recipe in theR4entry ofdocs/design/decisions.md. Thephremoval is classified as a 2.x parity break on the bare-ph-with-no-D.class (John Smith Ph.→ lastPh.), pinned by atests/v2/cases.pyrow and a rule in all five ledgers. Full suite: 9675 passed, 324 skipped, 4 xfailed; differential gate exit 0 at all five baselines, 0 unexplained.🤖 Generated with Claude Code