Address multi-agent review findings on PR #286

- Fix conftest.py's empty_attribute_default fixture teardown: the
  DeprecationWarning-ignore filter wrapped the restore of all 8 scalar
  config attrs instead of just the one deprecated assignment, which
  would have silently swallowed any future deprecation warning added
  to the other 7 -- a silent-failure hunter finding. Scoped to match
  the already-narrow setup block.
- Add a type check to _EmptyAttributeDefaultAttribute.__set__: a
  cheap, immediate TypeError for non-str/non-None values instead of
  deferring the whole invariant to 2.0 (type-design-analyzer finding).
- Document why __get__(obj=None) returns '' rather than `self` (unlike
  its _SetManagerAttribute sibling): it's load-bearing for
  Constants.__repr__'s default-value comparison, not just mypy
  inference (type-design-analyzer finding).
- Fix a stale "~8 public str-typed properties" comment (actually 12)
  in two test files (comment-analyzer finding).
- Add tests: Constants.copy() preserves a customized
  empty_attribute_default without warning; add_with_encoding()'s str
  path emits exactly one warning, not two; the legacy-pickle warning
  fires once per __setstate__ call even with two stale keys, not once
  per key (test-analyzer findings).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: derek73

nameparser/config/__init__.py

@@ -556,12 +556,22 @@ def __get__(self, obj: 'Constants | None', objtype: type | None = None) -> str:
         # Annotated `str`, not `str | None`, to match the pre-descriptor
         # plain-attribute inference: None is documented/supported (see the
         # class docstring), but typing it honestly cascades `| None`
-        # through ~8 public str-typed properties (title, first, ... ).
+        # through every public str-typed name accessor (title, first, ...).
+        # Returning '' rather than `self` on class access (unlike
+        # _SetManagerAttribute, which returns `self`) is also load-bearing
+        # for Constants.__repr__'s `getattr(type(self), name)` default
+        # comparison in _repr_scalar_attrs -- returning `self` there would
+        # make every Constants() show this attribute as "customized".
         if obj is None:
             return ''
         return getattr(obj, self._attr, '')
 
     def __set__(self, obj: 'Constants', value: str | None) -> None:
+        if value is not None and not isinstance(value, str):
+            raise TypeError(
+                f"empty_attribute_default must be a str or None, got "
+                f"{type(value).__name__!r}"
+            )
         warnings.warn(
             "Assigning Constants.empty_attribute_default is deprecated and "
             "will raise TypeError in 2.0; empty attributes will always "

tests/conftest.py

@@ -71,9 +71,12 @@ def empty_attribute_default(request: pytest.FixtureRequest) -> Iterator[str | No
         warnings.simplefilter("ignore", DeprecationWarning)
         CONSTANTS.empty_attribute_default = request.param
     yield request.param
-    with warnings.catch_warnings():
-        warnings.simplefilter("ignore", DeprecationWarning)
-        for attr, value in scalar_snapshot.items():
+    for attr, value in scalar_snapshot.items():
+        if attr == "empty_attribute_default":
+            with warnings.catch_warnings():
+                warnings.simplefilter("ignore", DeprecationWarning)
+                setattr(CONSTANTS, attr, value)
+        else:
             setattr(CONSTANTS, attr, value)
     for attr, value in collection_snapshot.items():
         setattr(CONSTANTS, attr, value)

tests/test_constants.py

@@ -309,6 +309,16 @@ def test_empty_attribute_default_assignment_emits_deprecation_warning(self) -> N
             c.empty_attribute_default = None  # type: ignore[assignment]
         self.assertIsNone(c.empty_attribute_default)
 
+    def test_empty_attribute_default_rejects_non_str_non_none(self) -> None:
+        # A cheap early check on the invariant 2.0 will fully enforce (only
+        # '' will be legal): non-str/non-None values fail loudly here
+        # instead of surfacing later as a confusing failure deep in some
+        # unrelated HumanName string property.
+        c = Constants()
+        with pytest.raises(TypeError, match="str or None"):
+            c.empty_attribute_default = 42  # type: ignore[assignment]
+        self.assertEqual(c.empty_attribute_default, '')
+
     def test_empty_attribute_default_read_does_not_warn(self) -> None:
         c = Constants()
         with warnings.catch_warnings():
@@ -321,8 +331,9 @@ def test_empty_attribute_default(self) -> None:
         # from the '' default), but None is documented/supported here -- see
         # the doctest on the attribute's docstring in config/__init__.py.
         # Not widened to str | None like string_format/suffix_delimiter
-        # because it cascades into ~8 public str-typed properties (title,
-        # first, middle, last, suffix, nickname, initials()).
+        # because it cascades into every public str-typed name accessor
+        # (title, first, middle, last, suffix, nickname, maiden, surnames,
+        # given_names, last_base, last_prefixes, initials()).
         with pytest.deprecated_call():
             CONSTANTS.empty_attribute_default = None  # type: ignore[assignment]
         hn = HumanName("")
@@ -369,8 +380,14 @@ def test_add_with_encoding_str_emits_deprecation_warning(self) -> None:
         # (#263/#245); the str path is otherwise silent, so this method's
         # own removal is unwarned without this
         sm = SetManager(['dr'])
-        with pytest.deprecated_call(match="add\\(\\)"):
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter("always")
             sm.add_with_encoding('esq')
+        deprecations = [w for w in caught if issubclass(w.category, DeprecationWarning)]
+        # Exactly one -- the str path must not also trip the bytes-decode
+        # warning (that one's for the *other* argument type).
+        self.assertEqual(len(deprecations), 1)
+        self.assertIn('add()', str(deprecations[0].message))
         self.assertIn('esq', sm)
 
     def test_add_with_encoding_bytes_emits_two_distinct_warnings(self) -> None:
@@ -531,6 +548,30 @@ def test_unpickle_legacy_state_emits_deprecation_warning_once(self) -> None:
         deprecations = [w for w in record.list if issubclass(w.category, DeprecationWarning)]
         self.assertEqual(len(deprecations), 1)
 
+    def test_unpickle_legacy_state_with_two_stale_property_keys_warns_once(self) -> None:
+        # Pins "once per __setstate__ call, not once per skipped key": with
+        # only one computed property (suffixes_prefixes_titles) on the real
+        # Constants class, the test above can't distinguish the two
+        # semantics. A second read-only property on a throwaway subclass
+        # forces two keys through the skip branch in one __setstate__ call.
+        class ConstantsWithExtraProperty(Constants):
+            @property
+            def another_computed_property(self) -> str:
+                return 'computed'
+
+        c = ConstantsWithExtraProperty()
+        legacy_state = {
+            name: getattr(c, name) for name in dir(c) if not name.startswith('_')
+        }
+        self.assertIn('suffixes_prefixes_titles', legacy_state)
+        self.assertIn('another_computed_property', legacy_state)
+
+        restored = ConstantsWithExtraProperty.__new__(ConstantsWithExtraProperty)
+        with pytest.deprecated_call(match="re-pickle") as record:
+            restored.__setstate__(legacy_state)
+        deprecations = [w for w in record.list if issubclass(w.category, DeprecationWarning)]
+        self.assertEqual(len(deprecations), 1)
+
     def test_setstate_without_legacy_keys_does_not_warn(self) -> None:
         c = Constants()
         c.titles.add('legacytitle')
@@ -1134,6 +1175,20 @@ class CustomConstants(Constants):
         dup = c.copy()
         self.assertTrue(isinstance(dup, CustomConstants))
 
+    def test_copy_preserves_empty_attribute_default_without_warning(self) -> None:
+        # copy() round-trips through __getstate__/__setstate__ like pickle
+        # does; restoring saved state isn't a user assignment, so it must not
+        # emit #255's deprecation warning (the __setstate__ bypass exists
+        # specifically for this call path, not just pickle.loads).
+        c = Constants()
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore", DeprecationWarning)
+            c.empty_attribute_default = None  # type: ignore[assignment]
+        with warnings.catch_warnings():
+            warnings.simplefilter("error")
+            dup = c.copy()
+        self.assertIsNone(dup.empty_attribute_default)
+
     def test_copy_snapshots_current_customizations(self) -> None:
         # Unlike Constants(), which always starts from library defaults,
         # .copy() preserves whatever customizations the original already has.

tests/test_initials.py

@@ -32,8 +32,9 @@ def test_initials_empty_part_with_none_default_not_literal_none(self) -> None:
         # from the '' default), but None is documented/supported here -- see
         # the doctest on the attribute's docstring in config/__init__.py. Not
         # widened to str | None like string_format/suffix_delimiter because
-        # it cascades into ~8 public str-typed properties (title, first,
-        # middle, last, suffix, nickname, initials()).
+        # it cascades into every public str-typed name accessor (title,
+        # first, middle, last, suffix, nickname, maiden, surnames,
+        # given_names, last_base, last_prefixes, initials()).
         with pytest.deprecated_call():
             hn.C.empty_attribute_default = None  # type: ignore[assignment]
         self.assertEqual(hn.initials(), "J. D.")