Skip to content

fix: rename _get_handlers to get_default_handlers and improve handler… #38

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bybatkhuu wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: rename _get_handlers to get_default_handlers and improve handler… #38

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 26 additions & 20 deletions src/beans_logging/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
from .schemas import ExtraBaseModel, LogHandlerPM, LoguruHandlerPM


def _get_handlers() -> dict[str, LogHandlerPM]:
def get_default_handlers() -> dict[str, LogHandlerPM]:
"""Get default log handlers.

Returns:
Expand Down Expand Up @@ -131,36 +131,42 @@ class LoggerConfigPM(ExtraBaseModel):
)
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)
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Removing the default_factory since the validator now handles the default case
  2. 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.

Suggested change
handlers: dict[str, LogHandlerPM] = Field(default_factory=get_default_handlers)
handlers: dict[str, LogHandlerPM] = Field()

Copilot uses AI. Check for mistakes.
extra: ExtraConfigPM | None = Field(default_factory=ExtraConfigPM)

@field_validator("handlers", mode="before")
@classmethod
def _check_handlers(cls, val: Any) -> Any:
if val:
if not isinstance(val, dict):
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}
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Document this breaking change in the CHANGELOG or release notes
  2. Consider if this is the intended behavior - users may want to completely replace handlers, not just merge with defaults
  3. If merging is desired, consider adding an option to allow users to opt out of including defaults

Copilot uses AI. Check for mistakes.
return val
Comment on lines +139 to 166
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new behavior where user-provided handlers are always merged with default handlers is not covered by tests. Consider adding tests that verify:

  1. When no handlers are provided, all defaults are present
  2. When user provides handlers with same keys as defaults, user values override defaults
  3. When user provides handlers with different keys, both defaults and user handlers are present
  4. 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.

Copilot uses AI. Check for mistakes.


__all__ = [
"LoggerConfigPM",
"get_default_handlers",
Copy link

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
"get_default_handlers",

Copilot uses AI. Check for mistakes.
]
Loading