From c37c4e7b1c3c1477c9621eab149e4a082add4621 Mon Sep 17 00:00:00 2001 From: M Bussonnier Date: Mon, 13 Jul 2026 09:47:50 +0200 Subject: [PATCH] Fix three correctness bugs found in code review - HasTraits.class_own_trait_events called the nonexistent cls.events() and raised AttributeError on every call; the method had clearly never worked (nothing in the test suite exercised it). Call trait_events() instead, and fix the docstring, which pointed at an equally nonexistent ``event_handlers`` method. Also remove the unreachable 'tags' branch in trait_events: it guarded on hasattr(v, "tags"), but no EventHandler ever gets a .tags attribute -- tags are trait metadata, not handler attributes -- so the branch could never run. - Application.flatten_flags checked isinstance(aliases, tuple), i.e. the output dict being built (never a tuple), instead of the loop variable `alias`. mypy had flagged the branch as unreachable and the warning was silenced with a type:ignore rather than investigated. As a result tuple-valued alias keys like ("f", "foo") were never exploded into individual names, so the collision check between flags and aliases missed them, and a flag sharing a name with one member of a tuple alias crashed argparse with 'conflicting option strings' instead of merging into an optional-argument alias. - get_logger() permanently cached whichever logger it returned first. If it was first called before any Application existed (easy to hit: any config-file load warning does it), the fallback library logger was cached and the Application's configured logger was never returned afterward, silently dropping app log output routed through traitlets.log. Check Application.initialized() on every call instead. With that fix the module-level _logger cache no longer served any purpose -- logging.getLogger() already caches by name -- so remove the mutable global entirely and attach the NullHandler once at import time, per stdlib best practice for libraries. A side benefit of not caching: after Application.clear_instance(), callers fall back to the library logger instead of a stale reference to the torn-down app's. - Remove traitlets/tests/test_traitlets.py: a stale duplicate of the TraitTestBase helper that shipped inside the installed package. The real copy (actually used by the suite) lives in tests/test_traitlets.py. Add regression tests for all three fixes: trait_events / class_own_trait_events inheritance behavior, tuple-alias flattening and flag/alias name collision, and get_logger picking up an Application created after the first call. Co-Authored-By: Claude Fable 5 --- tests/config/test_application.py | 44 +++++++++++++++++++++++ tests/test_traitlets.py | 38 ++++++++++++++++++++ traitlets/config/application.py | 4 +-- traitlets/log.py | 23 +++++------- traitlets/tests/test_traitlets.py | 59 ------------------------------- traitlets/traitlets.py | 11 ++---- 6 files changed, 95 insertions(+), 84 deletions(-) delete mode 100644 traitlets/tests/test_traitlets.py diff --git a/tests/config/test_application.py b/tests/config/test_application.py index a3044dd8..a3f9346f 100644 --- a/tests/config/test_application.py +++ b/tests/config/test_application.py @@ -466,6 +466,33 @@ def test_flatten_aliases(self): # this would be app.config.Application.log_level if it failed: self.assertEqual(app.config.MyApp.log_level, "CRITICAL") + def test_flatten_aliases_tuple_keys(self): + # tuple alias keys must be exploded into one entry per name, so + # that the loader can detect collisions between flags and aliases + app = MyApp() + flags, aliases = app.flatten_flags() + self.assertEqual(aliases["fooi"], "Foo.i") + self.assertEqual(aliases["i"], "Foo.i") + self.assertNotIn(("fooi", "i"), aliases) + + def test_flag_tuple_alias_collision(self): + # a flag sharing a name with one member of a tuple alias used to + # crash argparse with 'conflicting option strings' + class CollisionApp(Application): + classes = List([Bar]) # type:ignore[assignment] + aliases = {("b", "bee"): "Bar.b"} + flags = {"b": ({"Bar": {"enabled": False}}, "Disable Bar")} + + # with an argument it acts as the alias + app = CollisionApp() + app.parse_command_line(["-b", "5"]) + self.assertEqual(app.config.Bar.b, 5) + + # without an argument it acts as the flag + app = CollisionApp() + app.parse_command_line(["-b"]) + self.assertEqual(app.config.Bar.enabled, False) + def test_extra_args(self): app = MyApp() app.parse_command_line(["--Bar.b=5", "extra", "args", "--disable"]) @@ -899,6 +926,23 @@ def test_logging_teardown_on_error(capsys, caplogconfig): assert len(caplogconfig) == 1 # logging was configured +def test_get_logger_after_application(): + # get_logger must pick up the Application's logger even if it was + # first called (returning the fallback) before the Application existed, + # and fall back again once the Application is torn down + import traitlets.log + + Application.clear_instance() + try: + fallback = traitlets.log.get_logger() + assert fallback.name == "traitlets" + app = Application.instance() + assert traitlets.log.get_logger() is app.log + finally: + Application.clear_instance() + assert traitlets.log.get_logger() is fallback + + if __name__ == "__main__": # for test_help_output: MyApp.launch_instance() diff --git a/tests/test_traitlets.py b/tests/test_traitlets.py index 60b790a9..ce480718 100644 --- a/tests/test_traitlets.py +++ b/tests/test_traitlets.py @@ -824,6 +824,44 @@ class A(HasTraits): self.assertTrue(a.has_trait("f")) self.assertFalse(a.has_trait("g")) + def test_trait_events(self): + class A(HasTraits): + i = Int() + + @observe("i") + def _on_i(self, change): + pass + + class B(A): + f = Float() + + @observe("f") + def _on_f(self, change): + pass + + self.assertEqual(sorted(B.trait_events()), ["_on_f", "_on_i"]) + self.assertEqual(list(B.trait_events("f")), ["_on_f"]) + self.assertEqual(list(B.trait_events("i")), ["_on_i"]) + + def test_class_own_trait_events(self): + class A(HasTraits): + i = Int() + + @observe("i") + def _on_i(self, change): + pass + + class B(A): + f = Float() + + @observe("f") + def _on_f(self, change): + pass + + self.assertEqual(list(A.class_own_trait_events("i")), ["_on_i"]) + self.assertEqual(list(B.class_own_trait_events("f")), ["_on_f"]) + self.assertEqual(list(B.class_own_trait_events("i")), []) + def test_trait_has_value(self): class A(HasTraits): i = Int() diff --git a/traitlets/config/application.py b/traitlets/config/application.py index 1d475c58..d9a49a60 100644 --- a/traitlets/config/application.py +++ b/traitlets/config/application.py @@ -753,8 +753,8 @@ def flatten_flags(self) -> tuple[dict[str, t.Any], dict[str, t.Any]]: if len(children) == 1: # exactly one descendent, promote alias cls = children[0] # type:ignore[assignment] - if not isinstance(aliases, tuple): # type:ignore[unreachable] - alias = (alias,) # type:ignore[assignment] + if not isinstance(alias, tuple): + alias = (alias,) for al in alias: aliases[al] = ".".join([cls, trait]) # type:ignore[list-item] diff --git a/traitlets/log.py b/traitlets/log.py index 112b2004..704899ea 100644 --- a/traitlets/log.py +++ b/traitlets/log.py @@ -7,25 +7,20 @@ import logging from typing import Any -_logger: logging.Logger | logging.LoggerAdapter[Any] | None = None +# Add a NullHandler to silence warnings about not being +# initialized, per best practice for libraries. +_fallback = logging.getLogger("traitlets") +_fallback.addHandler(logging.NullHandler()) def get_logger() -> logging.Logger | logging.LoggerAdapter[Any]: """Grab the global logger instance. If a global Application is instantiated, grab its logger. - Otherwise, grab the root logger. + Otherwise, grab the 'traitlets' library logger. """ - global _logger # noqa: PLW0603 + from .config import Application - if _logger is None: - from .config import Application - - if Application.initialized(): - _logger = Application.instance().log - else: - _logger = logging.getLogger("traitlets") - # Add a NullHandler to silence warnings about not being - # initialized, per best practice for libraries. - _logger.addHandler(logging.NullHandler()) - return _logger + if Application.initialized(): + return Application.instance().log + return _fallback diff --git a/traitlets/tests/test_traitlets.py b/traitlets/tests/test_traitlets.py deleted file mode 100644 index 8380059f..00000000 --- a/traitlets/tests/test_traitlets.py +++ /dev/null @@ -1,59 +0,0 @@ -from __future__ import annotations - -from typing import Any -from unittest import TestCase - -from traitlets import TraitError - - -class TraitTestBase(TestCase): - """A best testing class for basic trait types.""" - - def assign(self, value: Any) -> None: - self.obj.value = value # type:ignore[attr-defined] - - def coerce(self, value: Any) -> Any: - return value - - def test_good_values(self) -> None: - if hasattr(self, "_good_values"): - for value in self._good_values: - self.assign(value) - self.assertEqual(self.obj.value, self.coerce(value)) # type:ignore[attr-defined] - - def test_bad_values(self) -> None: - if hasattr(self, "_bad_values"): - for value in self._bad_values: - try: - self.assertRaises(TraitError, self.assign, value) - except AssertionError: - raise AssertionError(value) from None - - def test_default_value(self) -> None: - if hasattr(self, "_default_value"): - self.assertEqual(self._default_value, self.obj.value) # type:ignore[attr-defined] - - def test_allow_none(self) -> None: - if ( - hasattr(self, "_bad_values") - and hasattr(self, "_good_values") - and None in self._bad_values - ): - trait = self.obj.traits()["value"] # type:ignore[attr-defined] - try: - trait.allow_none = True - self._bad_values.remove(None) - # skip coerce. Allow None casts None to None. - self.assign(None) - self.assertEqual(self.obj.value, None) # type:ignore[attr-defined] - self.test_good_values() - self.test_bad_values() - finally: - # tear down - trait.allow_none = False - self._bad_values.append(None) - - def tearDown(self) -> None: - # restore default value after tests, if set - if hasattr(self, "_default_value"): - self.obj.value = self._default_value # type:ignore[attr-defined] diff --git a/traitlets/traitlets.py b/traitlets/traitlets.py index 9c33d2f3..ba6d93d3 100644 --- a/traitlets/traitlets.py +++ b/traitlets/traitlets.py @@ -1967,14 +1967,10 @@ def trait_metadata(self, traitname: str, key: str, default: t.Any = None) -> t.A def class_own_trait_events(cls: type[HasTraits], name: str) -> dict[str, EventHandler]: """Get a dict of all event handlers defined on this class, not a parent. - Works like ``event_handlers``, except for excluding traits from parents. + Works like ``trait_events``, except for excluding traits from parents. """ sup = super(cls, cls) - return { - n: e - for (n, e) in cls.events(name).items() # type:ignore[attr-defined] - if getattr(sup, n, None) is not e - } + return {n: e for (n, e) in cls.trait_events(name).items() if getattr(sup, n, None) is not e} @classmethod def trait_events(cls: type[HasTraits], name: str | None = None) -> dict[str, EventHandler]: @@ -1997,9 +1993,6 @@ def trait_events(cls: type[HasTraits], name: str | None = None) -> dict[str, Eve events[k] = v elif name in v.trait_names: # type:ignore[attr-defined] events[k] = v - elif hasattr(v, "tags"): - if cls.trait_names(**v.tags): - events[k] = v return events