Simplify pass over the changes since v1.2.1#204
Merged
Merged
Conversation
Behavior-preserving cleanups from a four-angle review (reuse, simplification, efficiency, altitude) of everything since v1.2.1: - join_on_conjunctions: share one register_joined_piece helper for the title/prefix registration duplicated across both join branches - Consolidate the lenient post-comma suffix rule into is_suffix_lenient(); are_suffixes_after_comma uses it, and the single-use is_suffix_at_lastname_comma_end method is inlined at its only call site - cap_word: compute the exception-lookup key once instead of up to four lc()/replace() calls per word - Constants.__setstate__: drop the verification loop that could never fire for state produced by __getstate__ - Extract _is_dunder() for the guard copy-pasted across four TupleManager/RegexTupleManager attribute hooks - tests: add FlaggedConstantsTestBase replacing seven copy-pasted setup_method/hn() fixture pairs; drop the two invariant tests that duplicate prefixes.py's import-time asserts Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review of #204 flagged the __setstate__ verification loop drop as an undisclosed diagnostic regression (truncated/incompatible pickle state would surface as a confusing AttributeError instead of a clear ValueError), so it's restored. Also trims the two __getattr__ comments that duplicated _is_dunder()'s rationale instead of stating only the site-specific consequence. Co-Authored-By: Claude Sonnet 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.
What changed
A
/simplifyreview (four parallel angles: reuse, simplification, efficiency, altitude) over everything since the v1.2.1 tag, with the surviving findings applied. All changes are behavior-preserving — net −34 lines.nameparser/parser.pyjoin_on_conjunctions: both join branches carried identical 5-line title/prefix registration blocks (including the pre-existing title duplication); they now share one localregister_joined_piecehelper.suffix_not_acronymsmembers bypass theis_an_initialveto" rule was encoded twice; it now lives in one new predicate,is_suffix_lenient().are_suffixes_after_commais a one-lineall()over it, and the single-use 20-lineis_suffix_at_lastname_comma_endmethod is deleted, with its positional guards (nxt is None and len(parts) == 2) inlined at the lastname-comma loop that owns that context.cap_word: computes its exception-lookup key once instead of up to fourlc()/.replace()string allocations per word (runs per word-match on everycapitalize()pass).nameparser/config/__init__.pyConstants.__setstate__: dropped the 10-line verification loop — its condition can never fire for any state__getstate__produces, and removing it also eliminated the duplicated descriptor-scan between the two pickle hooks.TupleManager/RegexTupleManagerattribute hooks is now one_is_dunder()helper carrying the shared rationale; site-specific failure notes stay at each hook.Tests
FlaggedConstantsTestBase(with aconstants_kwargsclass attribute) replaces the identicalsetup_method/hn()pair copy-pasted into 7 classes across the patronymic-order and middle-name-as-last test files.test_prefixes.pyinvariant tests that duplicate the import-time asserts inprefixes.py— a violated assert prevents import entirely, so the tests could never be the thing that fails.AGENTS.md's note on the suffix/initial tension is updated to describe
is_suffix_lenient().Reviewer notes
parse_full_name—"Abdul Salam Hassan, MD"parses differently from"Abdul Salam Hassan MD". Tracked separately rather than folded into this cleanup PR.suffix_delimitersplit to suffix-classification time (design change), and extractingparse_nicknames' intentionally-inlined suffix normalization (the shared version came out clunkier than the commented inline form).Verification
uv run pytest: 1236 passed, 22 xfailed (same xfail count as before)uv run mypy nameparser/: clean; tests-included mypy error count identical to the pre-change baseline (41)🤖 Generated with Claude Code