Skip to content

Fix three correctness bugs found in code review by claude Fable#946

Draft
Carreau wants to merge 1 commit into
ipython:mainfrom
Carreau:f1
Draft

Fix three correctness bugs found in code review by claude Fable#946
Carreau wants to merge 1 commit into
ipython:mainfrom
Carreau:f1

Conversation

@Carreau

@Carreau Carreau commented Jul 13, 2026

Copy link
Copy Markdown
Member
  • 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.

- 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 <noreply@anthropic.com>
@Carreau

Carreau commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

from traitlets.tests.test_traitlets import TraitTestBase is used by ipywidgets, either we deduplicate in the other direction; or fix / make a duplicate in ipywidgets.

@Carreau Carreau marked this pull request as draft July 13, 2026 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant