Skip to content

Commit a42f71b

Browse files
committed
Make HumanName.C a property so post-construction assignment is validated
#226 validated the `constants` argument in HumanName.__init__, but `C` was a plain attribute, so `hn.C = 'garbage'` bypassed validation entirely and only surfaced as an unrelated AttributeError deep inside parsing. Extract _validate_constants as a shared staticmethod used by both the constructor and the new C property setter, so both paths raise the same immediate TypeError. __getstate__/__setstate__ are updated to translate the internal _C storage attribute to/from the same 'C' key they always pickled, so the on-disk pickle format (and the CONSTANTS-singleton sentinel) is unchanged. Fixes #239
1 parent 4d06435 commit a42f71b

2 files changed

Lines changed: 61 additions & 24 deletions

File tree

nameparser/parser.py

Lines changed: 41 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -68,14 +68,6 @@ class HumanName:
6868
:param str maiden: Maiden name
6969
"""
7070

71-
C = CONSTANTS
72-
"""
73-
A reference to the configuration for this instance, which may or may not be
74-
a reference to the shared, module-wide instance at
75-
:py:mod:`~nameparser.config.CONSTANTS`. See `Customizing the Parser
76-
<customize.html>`_.
77-
"""
78-
7971
original: str | bytes = ''
8072
"""
8173
The original string, untouched by the parser.
@@ -111,17 +103,6 @@ def __init__(
111103
nickname: str | list[str] | None = None,
112104
maiden: str | list[str] | None = None,
113105
) -> None:
114-
if constants is None:
115-
constants = Constants()
116-
elif not isinstance(constants, Constants):
117-
# passing the class itself is the likeliest mistake, and
118-
# reporting it as "got type" would only add confusion
119-
hint = (" (a class was passed; did you mean Constants()?)"
120-
if isinstance(constants, type) else "")
121-
raise TypeError(
122-
"constants must be a Constants instance or None, "
123-
f"got {type(constants).__name__}{hint}"
124-
)
125106
self.C = constants
126107

127108
# Lookup entries derived while parsing this instance (period-joined
@@ -156,15 +137,52 @@ def __init__(
156137
# full_name setter triggers the parse
157138
self.full_name = full_name
158139

140+
@staticmethod
141+
def _validate_constants(constants: 'Constants | None') -> 'Constants':
142+
# Shared by the constructor and the C setter so both assignment paths
143+
# give the same immediate TypeError instead of one bypassing the
144+
# other and failing far from the cause (#239).
145+
if constants is None:
146+
return Constants()
147+
if not isinstance(constants, Constants):
148+
# passing the class itself is the likeliest mistake, and
149+
# reporting it as "got type" would only add confusion
150+
hint = (" (a class was passed; did you mean Constants()?)"
151+
if isinstance(constants, type) else "")
152+
raise TypeError(
153+
"constants must be a Constants instance or None, "
154+
f"got {type(constants).__name__}{hint}"
155+
)
156+
return constants
157+
158+
@property
159+
def C(self) -> 'Constants':
160+
"""
161+
A reference to the configuration for this instance, which may or may not be
162+
a reference to the shared, module-wide instance at
163+
:py:mod:`~nameparser.config.CONSTANTS`. See `Customizing the Parser
164+
<customize.html>`_.
165+
166+
Assigning a non-``Constants`` value (besides ``None``, which builds a
167+
fresh private ``Constants()``) raises the same ``TypeError`` as passing
168+
an invalid ``constants`` argument to the constructor (#239).
169+
"""
170+
return self._C
171+
172+
@C.setter
173+
def C(self, constants: 'Constants | None') -> None:
174+
self._C = self._validate_constants(constants)
175+
159176
def __getstate__(self) -> dict:
160177
state = self.__dict__.copy()
161-
if state.get('C') is CONSTANTS:
162-
state['C'] = None # sentinel: restore shared singleton on load
178+
c = state.pop('_C')
179+
state['C'] = None if c is CONSTANTS else c # sentinel: restore shared singleton on load
163180
return state
164181

165182
def __setstate__(self, state: dict) -> None:
166-
if state.get('C') is None:
167-
state['C'] = CONSTANTS
183+
state = dict(state)
184+
c = state.pop('C', None)
185+
self._C = CONSTANTS if c is None else c
168186
self.__dict__.update(state)
169187
# pickles from before the per-parse derived sets existed lack them;
170188
# backfill so the is_* predicates work without a re-parse

tests/test_constants.py

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import pytest
77

88
from nameparser import HumanName
9-
from nameparser.config import Constants, RegexTupleManager, SetManager, TupleManager
9+
from nameparser.config import CONSTANTS, Constants, RegexTupleManager, SetManager, TupleManager
1010
from nameparser.config.regexes import EMPTY_REGEX
1111
from nameparser.config.titles import TITLES
1212

@@ -46,6 +46,25 @@ def test_constants_class_instead_of_instance_raises_with_hint(self) -> None:
4646
with pytest.raises(TypeError, match=r"did you mean Constants\(\)"):
4747
HumanName("John Doe", constants=Constants) # type: ignore[arg-type]
4848

49+
def test_assigning_invalid_constants_after_construction_raises(self) -> None:
50+
# #226 validated the constructor's `constants` argument, but `hn.C = ...`
51+
# bypassed it entirely: the bad value was accepted silently and only
52+
# surfaced far later, deep inside parsing, with no mention of `C` (#239)
53+
hn = HumanName("John Doe")
54+
with pytest.raises(TypeError, match="constants must be"):
55+
hn.C = "garbage" # type: ignore[assignment]
56+
57+
def test_assigning_constants_class_after_construction_raises_with_hint(self) -> None:
58+
hn = HumanName("John Doe")
59+
with pytest.raises(TypeError, match=r"did you mean Constants\(\)"):
60+
hn.C = Constants # type: ignore[assignment]
61+
62+
def test_assigning_none_to_constants_after_construction_builds_new_instance(self) -> None:
63+
hn = HumanName("John Doe")
64+
hn.C = None
65+
self.assertIsNot(hn.C, CONSTANTS)
66+
self.assertTrue(isinstance(hn.C, Constants))
67+
4968
def test_constants_bare_string_kwarg_raises_typeerror(self) -> None:
5069
# a bare string is an iterable of its characters, so set('dr') would
5170
# silently replace the default titles with {'d', 'r'} (#238); the

0 commit comments

Comments
 (0)