fix: rename _get_handlers to get_default_handlers and improve handler… #38
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request refactors the default log handler management by renaming _get_handlers to get_default_handlers, making it public, and changing the behavior to always merge user-provided handlers with defaults. The changes improve code clarity but introduce a significant behavioral change that affects how handlers are configured.
Changes:
- Renamed
_get_handlerstoget_default_handlersand made it publicly exportable - Updated
LoggerConfigPM.handlersfield to use the renamed function in its default_factory - Modified
_check_handlersvalidator to always merge user-provided handlers with defaults, ensuring all default handlers are present unless explicitly overridden
Comments suppressed due to low confidence (1)
src/beans_logging/config.py:17
- The docstring for get_default_handlers states "Get default log handlers" but doesn't explain what these defaults are or when this function should be called by external users (since it's now exported). Consider enhancing the docstring to include:
- A brief description of what the default handlers are
- When/why external users might want to call this function
- An example of the returned structure
This is especially important since the function is now part of the public API (exported in all).
"""Get default log handlers.
Returns:
dict[str, LogHandlerPM]: Default handlers as dictionary.
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| __all__ = [ | ||
| "LoggerConfigPM", | ||
| "get_default_handlers", |
There was a problem hiding this comment.
The function get_default_handlers is now exported in the config module's all list, but it's not re-exported in the main package init.py. This means users cannot import it as from beans_logging import get_default_handlers, but must use from beans_logging.config import get_default_handlers.
If this function is intended to be part of the public API (which the export suggests), consider adding it to src/beans_logging/init.py's imports and all list for easier access. Otherwise, if it's only meant for internal use within the config module, the export may be unnecessary.
| "get_default_handlers", |
| elif isinstance(_handler, dict): | ||
| val[_i] = LogHandlerPM(**_handler) | ||
|
|
||
| val = {**_default_handlers, **val} |
There was a problem hiding this comment.
The validator now always merges user-provided handlers with default handlers, which is a breaking behavioral change. Previously, if a user provided any handlers, only those handlers would be used. Now, all default handlers will always be included, with user-provided handlers overriding matching keys. This means users who previously provided a minimal set of handlers will now get all default handlers in addition to their custom ones.
Consider:
- Document this breaking change in the CHANGELOG or release notes
- Consider if this is the intended behavior - users may want to completely replace handlers, not just merge with defaults
- If merging is desired, consider adding an option to allow users to opt out of including defaults
| default: DefaultConfigPM = Field(default_factory=DefaultConfigPM) | ||
| intercept: InterceptConfigPM = Field(default_factory=InterceptConfigPM) | ||
| handlers: dict[str, LogHandlerPM] = Field(default_factory=_get_handlers) | ||
| handlers: dict[str, LogHandlerPM] = Field(default_factory=get_default_handlers) |
There was a problem hiding this comment.
With the validator using mode="before", when a user doesn't provide handlers, the validator will receive None or an undefined value (not the result of default_factory), check "if not val" at line 142, and return _default_handlers. This means the default_factory=get_default_handlers on line 134 will never actually be invoked, making it redundant.
In Pydantic v2, validators with mode="before" execute before default values are applied, so the default_factory is bypassed when the validator returns a value for a missing field.
Consider either:
- Removing the default_factory since the validator now handles the default case
- Changing the validator to mode="after" if you want both the factory and validator to run, though this would change the merging logic
The current implementation works correctly but has unnecessary redundancy.
| handlers: dict[str, LogHandlerPM] = Field(default_factory=get_default_handlers) | |
| handlers: dict[str, LogHandlerPM] = Field() |
| def _check_handlers(cls, val: Any) -> dict[str, LogHandlerPM]: | ||
| _default_handlers = get_default_handlers() | ||
|
|
||
| if not val: | ||
| return _default_handlers | ||
|
|
||
| if not isinstance(val, dict): | ||
| raise TypeError( | ||
| f"'handlers' attribute type {type(val).__name__} is invalid, must be a dict of <LogHandlerPM>, " | ||
| f"<LoguruHandlerPM> or dict!" | ||
| ) | ||
|
|
||
| for _i, _handler in val.items(): | ||
| if not isinstance(_handler, (LogHandlerPM, LoguruHandlerPM, dict)): | ||
| raise TypeError( | ||
| f"'handlers' attribute type {type(val).__name__} is invalid, must be a dict of <LogHandlerPM>, " | ||
| f"<LoguruHandlerPM> or dict!" | ||
| f"'handlers' attribute index {_i} type {type(_handler).__name__} is invalid, must be " | ||
| f"<LogHandlerPM>, <LoguruHandlerPM> or dict!" | ||
| ) | ||
|
|
||
| for _i, _handler in val.items(): | ||
| if not isinstance(_handler, (LogHandlerPM, LoguruHandlerPM, dict)): | ||
| raise TypeError( | ||
| f"'handlers' attribute index {_i} type {type(_handler).__name__} is invalid, must be " | ||
| f"<LogHandlerPM>, <LoguruHandlerPM> or dict!" | ||
| ) | ||
|
|
||
| if isinstance(_handler, LoguruHandlerPM): | ||
| val[_i] = LogHandlerPM( | ||
| **_handler.model_dump(exclude_none=True, exclude_unset=True) | ||
| ) | ||
| elif isinstance(_handler, dict): | ||
| val[_i] = LogHandlerPM(**_handler) | ||
| if isinstance(_handler, LoguruHandlerPM): | ||
| val[_i] = LogHandlerPM( | ||
| **_handler.model_dump(exclude_none=True, exclude_unset=True) | ||
| ) | ||
| elif isinstance(_handler, dict): | ||
| val[_i] = LogHandlerPM(**_handler) | ||
|
|
||
| val = {**_default_handlers, **val} | ||
| return val |
There was a problem hiding this comment.
The new behavior where user-provided handlers are always merged with default handlers is not covered by tests. Consider adding tests that verify:
- When no handlers are provided, all defaults are present
- When user provides handlers with same keys as defaults, user values override defaults
- When user provides handlers with different keys, both defaults and user handlers are present
- The newly exported get_default_handlers function works correctly
This is important to test since this is a significant behavioral change in how handlers are managed.
This pull request refactors the way default log handlers are managed in the logging configuration module. The changes improve clarity by renaming the internal function, updating its usage throughout the code, and ensuring that user-provided handlers are always merged with the defaults.
Refactoring of log handler defaults:
_get_handlerstoget_default_handlersfor better clarity and made it available for import.LoggerConfigPMclass to use the newget_default_handlersfunction as the default factory for thehandlersfield, replacing the old_get_handlers._check_handlersvalidator to always merge user-supplied handlers with the default handlers, ensuring all defaults are present unless explicitly overridden. [1] [2]get_default_handlersin the module’s__all__list to make it accessible for external use.