From 461bc049a058a233afe02c5ae3ba4a9571651e75 Mon Sep 17 00:00:00 2001 From: Sebastian Mohr Date: Wed, 17 Sep 2025 11:42:07 +0200 Subject: [PATCH 1/6] Enhanced custom logger typing and logging tests --- beets/logging.py | 47 ++++++++++++++++++++++++++++++-------------- test/test_logging.py | 45 +++++++++++++++++++++++++++++------------- 2 files changed, 63 insertions(+), 29 deletions(-) diff --git a/beets/logging.py b/beets/logging.py index fd8b1962f..becfff86f 100644 --- a/beets/logging.py +++ b/beets/logging.py @@ -20,6 +20,9 @@ use {}-style formatting and can interpolate keywords arguments to the logging calls (`debug`, `info`, etc). """ +from __future__ import annotations + +import logging import threading from copy import copy from logging import ( @@ -34,6 +37,9 @@ from logging import ( NullHandler, StreamHandler, ) +from typing import TYPE_CHECKING, Any, Mapping, TypeVar + +from typing_extensions import ParamSpec __all__ = [ "DEBUG", @@ -49,8 +55,14 @@ __all__ = [ "getLogger", ] +if TYPE_CHECKING: + T = TypeVar("T") -def logsafe(val): + +P = ParamSpec("P") + + +def _logsafe(val: T) -> str | T: """Coerce `bytes` to `str` to avoid crashes solely due to logging. This is particularly relevant for bytestring paths. Much of our code @@ -83,40 +95,45 @@ class StrFormatLogger(Logger): """ class _LogMessage: - def __init__(self, msg, args, kwargs): + def __init__( + self, + msg: str, + args: logging._ArgsType, + kwargs: dict[str, Any], + ): self.msg = msg self.args = args self.kwargs = kwargs def __str__(self): - args = [logsafe(a) for a in self.args] - kwargs = {k: logsafe(v) for (k, v) in self.kwargs.items()} + args = [_logsafe(a) for a in self.args] + kwargs = {k: _logsafe(v) for (k, v) in self.kwargs.items()} return self.msg.format(*args, **kwargs) def _log( self, - level, - msg, - args, - exc_info=None, - extra=None, - stack_info=False, + level: int, + msg: object, + args: logging._ArgsType = (), + exc_info: logging._ExcInfoType = None, + extra: Mapping[str, Any] | None = None, + stack_info: bool = False, + stacklevel: int = 1, **kwargs, ): """Log msg.format(*args, **kwargs)""" - m = self._LogMessage(msg, args, kwargs) - stacklevel = kwargs.pop("stacklevel", 1) - stacklevel = {"stacklevel": stacklevel} + if isinstance(msg, str): + msg = self._LogMessage(msg, args, kwargs) return super()._log( level, - m, + msg, (), exc_info=exc_info, extra=extra, stack_info=stack_info, - **stacklevel, + stacklevel=stacklevel, ) diff --git a/test/test_logging.py b/test/test_logging.py index b751dd70e..4a62e3a3b 100644 --- a/test/test_logging.py +++ b/test/test_logging.py @@ -3,10 +3,9 @@ import logging as log import sys import threading -import unittest -from io import StringIO from types import ModuleType -from unittest.mock import patch + +import pytest import beets.logging as blog from beets import plugins, ui @@ -14,8 +13,10 @@ from beets.test import _common, helper from beets.test.helper import AsIsImporterMixin, ImportTestCase, PluginMixin -class LoggingTest(unittest.TestCase): - def test_logging_management(self): +class TestStrFormatLogger: + """Tests for the custom str-formatting logger.""" + + def test_logger_creation(self): l1 = log.getLogger("foo123") l2 = blog.getLogger("foo123") assert l1 == l2 @@ -35,17 +36,33 @@ class LoggingTest(unittest.TestCase): l6 = blog.getLogger() assert l1 != l6 - def test_str_format_logging(self): - logger = blog.getLogger("baz123") - stream = StringIO() - handler = log.StreamHandler(stream) + @pytest.mark.parametrize( + "level", [log.DEBUG, log.INFO, log.WARNING, log.ERROR] + ) + @pytest.mark.parametrize( + "msg, args, kwargs, expected", + [ + ("foo {} bar {}", ("oof", "baz"), {}, "foo oof bar baz"), + ( + "foo {bar} baz {foo}", + (), + {"foo": "oof", "bar": "baz"}, + "foo baz baz oof", + ), + ("no args", (), {}, "no args"), + ("foo {} bar {baz}", ("oof",), {"baz": "baz"}, "foo oof bar baz"), + ], + ) + def test_str_format_logging( + self, level, msg, args, kwargs, expected, caplog + ): + logger = blog.getLogger("test_logger") + logger.setLevel(level) - logger.addHandler(handler) - logger.propagate = False + with caplog.at_level(level, logger="test_logger"): + logger.log(level, msg, *args, **kwargs) - logger.warning("foo {} {bar}", "oof", bar="baz") - handler.flush() - assert stream.getvalue(), "foo oof baz" + assert str(caplog.records[0].msg) == expected class DummyModule(ModuleType): From f637e5efbb29c47a3d1800e5c9233ecdea74a644 Mon Sep 17 00:00:00 2001 From: Sebastian Mohr Date: Fri, 19 Sep 2025 15:23:36 +0200 Subject: [PATCH 2/6] Added overload to getLogger function. Added changelog entry and added myself to codeowners file. --- .github/CODEOWNERS | 3 +++ beets/logging.py | 12 ++++++++---- docs/changelog.rst | 6 ++++++ 3 files changed, 17 insertions(+), 4 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 767509c9a..bb888d520 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,2 +1,5 @@ # assign the entire repo to the maintainers team * @beetbox/maintainers + +# Specific ownerships: +/beets/metadata_plugins.py @semohr \ No newline at end of file diff --git a/beets/logging.py b/beets/logging.py index becfff86f..29357c0f0 100644 --- a/beets/logging.py +++ b/beets/logging.py @@ -35,9 +35,10 @@ from logging import ( Handler, Logger, NullHandler, + RootLogger, StreamHandler, ) -from typing import TYPE_CHECKING, Any, Mapping, TypeVar +from typing import TYPE_CHECKING, Any, Mapping, TypeVar, overload from typing_extensions import ParamSpec @@ -173,9 +174,12 @@ my_manager = copy(Logger.manager) my_manager.loggerClass = BeetsLogger -# Override the `getLogger` to use our machinery. -def getLogger(name=None): # noqa +@overload +def getLogger(name: str) -> BeetsLogger: ... +@overload +def getLogger(name: None = ...) -> RootLogger: ... +def getLogger(name=None) -> BeetsLogger | RootLogger: # noqa: N802 if name: - return my_manager.getLogger(name) + return my_manager.getLogger(name) # type: ignore[return-value] else: return Logger.root diff --git a/docs/changelog.rst b/docs/changelog.rst index 52e935445..38037955e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -51,6 +51,12 @@ Other changes: - :class:`beets.metadata_plugin.MetadataSourcePlugin`: Remove discogs specific disambiguation stripping. +For developers and plugin authors: + +- Typing improvements in ``beets/logging.py``: ``getLogger`` now returns + ``BeetsLogger`` when called with a name, or ``RootLogger`` when called without + a name. + 2.4.0 (September 13, 2025) -------------------------- From b2fc007480f50278c44eb5e7c2ea13181822a0bf Mon Sep 17 00:00:00 2001 From: Sebastian Mohr Date: Fri, 19 Sep 2025 15:35:51 +0200 Subject: [PATCH 3/6] Fixed plugin typehints: use actual logger class. --- beetsplug/fetchart.py | 2 +- beetsplug/lyrics.py | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/beetsplug/fetchart.py b/beetsplug/fetchart.py index 54c065da4..37e7426f6 100644 --- a/beetsplug/fetchart.py +++ b/beetsplug/fetchart.py @@ -36,10 +36,10 @@ from beets.util.config import sanitize_pairs if TYPE_CHECKING: from collections.abc import Iterable, Iterator, Sequence - from logging import Logger from beets.importer import ImportSession, ImportTask from beets.library import Album, Library + from beets.logging import BeetsLogger as Logger try: from bs4 import BeautifulSoup, Tag diff --git a/beetsplug/lyrics.py b/beetsplug/lyrics.py index f492ab3cc..d245d6a14 100644 --- a/beetsplug/lyrics.py +++ b/beetsplug/lyrics.py @@ -42,10 +42,9 @@ from beets.autotag.distance import string_dist from beets.util.config import sanitize_choices if TYPE_CHECKING: - from logging import Logger - from beets.importer import ImportTask from beets.library import Item, Library + from beets.logging import BeetsLogger as Logger from ._typing import ( GeniusAPI, @@ -186,7 +185,7 @@ def slug(text: str) -> str: class RequestHandler: - _log: beets.logging.Logger + _log: Logger def debug(self, message: str, *args) -> None: """Log a debug message with the class name.""" From caebf185f11b24ab73ccbf0699e5078bccef6bcf Mon Sep 17 00:00:00 2001 From: Sebastian Mohr Date: Fri, 19 Sep 2025 15:39:43 +0200 Subject: [PATCH 4/6] Removed unused ParamSpec and added a consistency check in the tests. --- beets/logging.py | 5 ----- test/test_logging.py | 1 + 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/beets/logging.py b/beets/logging.py index 29357c0f0..086a590a0 100644 --- a/beets/logging.py +++ b/beets/logging.py @@ -40,8 +40,6 @@ from logging import ( ) from typing import TYPE_CHECKING, Any, Mapping, TypeVar, overload -from typing_extensions import ParamSpec - __all__ = [ "DEBUG", "INFO", @@ -60,9 +58,6 @@ if TYPE_CHECKING: T = TypeVar("T") -P = ParamSpec("P") - - def _logsafe(val: T) -> str | T: """Coerce `bytes` to `str` to avoid crashes solely due to logging. diff --git a/test/test_logging.py b/test/test_logging.py index 4a62e3a3b..f4dda331e 100644 --- a/test/test_logging.py +++ b/test/test_logging.py @@ -62,6 +62,7 @@ class TestStrFormatLogger: with caplog.at_level(level, logger="test_logger"): logger.log(level, msg, *args, **kwargs) + assert caplog.records, "No log records were captured" assert str(caplog.records[0].msg) == expected From 837295e2502a220fea0798f8352756d85627ba07 Mon Sep 17 00:00:00 2001 From: Sebastian Mohr Date: Sun, 21 Sep 2025 22:27:24 +0200 Subject: [PATCH 5/6] Added typehints from typeshed and removed default argument. --- beets/logging.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/beets/logging.py b/beets/logging.py index 086a590a0..64d6c50b1 100644 --- a/beets/logging.py +++ b/beets/logging.py @@ -22,7 +22,6 @@ calls (`debug`, `info`, etc). from __future__ import annotations -import logging import threading from copy import copy from logging import ( @@ -40,6 +39,8 @@ from logging import ( ) from typing import TYPE_CHECKING, Any, Mapping, TypeVar, overload +from typing_extensions import TypeAlias + __all__ = [ "DEBUG", "INFO", @@ -56,6 +57,15 @@ __all__ = [ if TYPE_CHECKING: T = TypeVar("T") + from types import TracebackType + + # see https://github.com/python/typeshed/blob/main/stdlib/logging/__init__.pyi + _SysExcInfoType: TypeAlias = ( + tuple[type[BaseException], BaseException, TracebackType | None] + | tuple[None, None, None] + ) + _ExcInfoType: TypeAlias = None | bool | _SysExcInfoType | BaseException + _ArgsType: TypeAlias = tuple[object, ...] | Mapping[str, object] def _logsafe(val: T) -> str | T: @@ -94,7 +104,7 @@ class StrFormatLogger(Logger): def __init__( self, msg: str, - args: logging._ArgsType, + args: _ArgsType, kwargs: dict[str, Any], ): self.msg = msg @@ -110,8 +120,8 @@ class StrFormatLogger(Logger): self, level: int, msg: object, - args: logging._ArgsType = (), - exc_info: logging._ExcInfoType = None, + args: _ArgsType, + exc_info: _ExcInfoType = None, extra: Mapping[str, Any] | None = None, stack_info: bool = False, stacklevel: int = 1, From 89c2e10680b56873a07738b0a745205391df7425 Mon Sep 17 00:00:00 2001 From: Sebastian Mohr Date: Sun, 21 Sep 2025 22:29:55 +0200 Subject: [PATCH 6/6] Removed typealias, worked locally with mypy but does seem to cause issues with the ci. Also python 3.9 requires unions here... --- beets/logging.py | 16 +++++++--------- test/test_logging.py | 1 + 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/beets/logging.py b/beets/logging.py index 64d6c50b1..3ed5e5a84 100644 --- a/beets/logging.py +++ b/beets/logging.py @@ -37,9 +37,7 @@ from logging import ( RootLogger, StreamHandler, ) -from typing import TYPE_CHECKING, Any, Mapping, TypeVar, overload - -from typing_extensions import TypeAlias +from typing import TYPE_CHECKING, Any, Mapping, TypeVar, Union, overload __all__ = [ "DEBUG", @@ -60,12 +58,12 @@ if TYPE_CHECKING: from types import TracebackType # see https://github.com/python/typeshed/blob/main/stdlib/logging/__init__.pyi - _SysExcInfoType: TypeAlias = ( - tuple[type[BaseException], BaseException, TracebackType | None] - | tuple[None, None, None] - ) - _ExcInfoType: TypeAlias = None | bool | _SysExcInfoType | BaseException - _ArgsType: TypeAlias = tuple[object, ...] | Mapping[str, object] + _SysExcInfoType = Union[ + tuple[type[BaseException], BaseException, Union[TracebackType, None]], + tuple[None, None, None], + ] + _ExcInfoType = Union[None, bool, _SysExcInfoType, BaseException] + _ArgsType = Union[tuple[object, ...], Mapping[str, object]] def _logsafe(val: T) -> str | T: diff --git a/test/test_logging.py b/test/test_logging.py index f4dda331e..48f9cbfd8 100644 --- a/test/test_logging.py +++ b/test/test_logging.py @@ -4,6 +4,7 @@ import logging as log import sys import threading from types import ModuleType +from unittest.mock import patch import pytest