Fix three correctness bugs found in code review by claude Fable#946
Draft
Carreau wants to merge 1 commit into
Draft
Fix three correctness bugs found in code review by claude Fable#946Carreau wants to merge 1 commit into
Carreau wants to merge 1 commit into
Conversation
- 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>
Member
Author
|
|
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.
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_handlersmethod. 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.