From d5220f7b75a06f7fffedd9cad431ebf9407701a6 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Sat, 12 Sep 2026 22:40:30 +0200 Subject: [PATCH 1/3] Add detail to traced values where str() is ambiguous Trace output stays str() based, because that is what makes it readable, but str() hides things a reader needs often enough to be worth fixing case by case: - an empty string is indistinguishable from no value at all, which is exactly wrong for a comparison trace showing left and right - a string carrying whitespace has no visible boundaries - an IntEnum prints as a bare number, losing the member name - a path prints as text, so a PosixPath and a py.path.local argument pointing at the same place look identical - a multi line value runs into column 0 and reads as a trace line of its own rather than as the value of its key So values that read unambiguously as themselves -- a non empty printable string without spaces, and every type whose repr adds nothing -- stay bare, and the rest gain quotes, their type, or a block. Multi line values are drawn as a box, each line prefixed with | and the last with \\, so the extent of the value is visible at a glance. Measured on a real pytest --debug run, this changes 45 of 1329 trace lines, against 216 for rendering every value with repr(). Every changed line carries information the previous rendering dropped. Co-Authored-By: Claude Opus 5 (1M context) Co-Authored-By: Claude Code --- docs/index.rst | 19 +++++++++++ src/pluggy/_tracing.py | 63 ++++++++++++++++++++++++++++++++--- testing/test_pluginmanager.py | 2 +- testing/test_tracer.py | 47 ++++++++++++++++++++++++-- 4 files changed, 123 insertions(+), 8 deletions(-) diff --git a/docs/index.rst b/docs/index.rst index 9d56f019..9ab83747 100644 --- a/docs/index.rst +++ b/docs/index.rst @@ -1030,6 +1030,25 @@ undo function to disable the behaviour. pm.trace.root.setwriter(print) undo = pm.enable_tracing() +Each hook call is traced with its keyword arguments, followed by a ``finish`` +line carrying the result:: + + he_method1 [hook] + plugin_name: example + path: PosixPath('/tmp') + reason: 'needs a network connection' + status: + explanation: + | first line + \ second line + finish he_method1 --> ['value'] [hook] + +Values are rendered with :func:`str` wherever that reads unambiguously, and with +:func:`repr` where it does not: an empty string, a string carrying whitespace, +an enum member, or a path, whose type is otherwise easy to lose. A value +spanning several lines is drawn as a block so that it stays attached to its key +instead of running into the surrounding trace. + Call monitoring --------------- diff --git a/src/pluggy/_tracing.py b/src/pluggy/_tracing.py index 3e01e8bf..b2044d16 100644 --- a/src/pluggy/_tracing.py +++ b/src/pluggy/_tracing.py @@ -6,6 +6,8 @@ from collections.abc import Callable from collections.abc import Sequence +import enum +import os from typing import Any @@ -13,13 +15,13 @@ _Processor = Callable[[tuple[str, ...], tuple[Any, ...]], object] -def _describe_str_failure(exc: Exception, obj: object) -> str: +def _describe_failure(exc: Exception, obj: object, func: str) -> str: try: exc_info = repr(exc) except Exception: exc_info = f"unpresentable {type(exc).__name__}" name = type(obj).__name__ - return f"<[{exc_info} raised in str()] {name} object at 0x{id(obj):x}>" + return f"<[{exc_info} raised in {func}()] {name} object at 0x{id(obj):x}>" def _escape_surrogates(text: str) -> str: @@ -42,7 +44,55 @@ def _safe_str(obj: object) -> str: try: text = str(obj) except Exception as exc: - text = _describe_str_failure(exc, obj) + text = _describe_failure(exc, obj, "str") + return _escape_surrogates(text) + + +def _is_plain_token(text: str) -> bool: + """Whether ``text`` can be shown bare, without quotes around it.""" + return bool(text) and text.isprintable() and " " not in text + + +def _format_block(indent: str, text: str) -> list[str]: + """Draw a multi line value as a box, so it reads as one value. + + The left edge marks every line as continuation, and the final ``\\`` + closes it, which keeps a block distinguishable from the trace lines + around it. + """ + body = text.split("\n") + edges = ["|"] * (len(body) - 1) + ["\\"] + return [f"{indent} {edge} {line}\n" for edge, line in zip(edges, body)] + + +def _render_value(obj: object) -> str: + """Render a traced value, adding detail only where ``str`` is ambiguous. + + Most values keep their plain ``str`` rendering, which is what makes a trace + readable. ``repr`` is used only where ``str`` hides something the reader + needs: the type of a path, the name of an enum member, or the boundaries of + a string that is empty or carries whitespace. + """ + if isinstance(obj, str): + if "\n" in obj or "\r" in obj: + return _safe_str(obj) + if _is_plain_token(obj): + return _safe_str(obj) + return _safe_repr(obj) + if isinstance(obj, (enum.Enum, os.PathLike)): + return _safe_repr(obj) + return _safe_str(obj) + + +def _safe_repr(obj: object) -> str: + """``repr(obj)`` for tracing, with a failing ``__repr__`` rendered, not raised. + + The result has lone surrogates escaped, so any text writer accepts it. + """ + try: + text = repr(obj) + except Exception as exc: + text = _describe_failure(exc, obj, "repr") return _escape_surrogates(text) @@ -68,7 +118,12 @@ def _format_message(self, tags: Sequence[str], args: Sequence[object]) -> str: lines = [f"{indent}{content} [{':'.join(tags)}]\n"] for name, value in extra.items(): - lines.append(f"{indent} {name}: {_safe_str(value)}\n") + rendered = _render_value(value) + if "\n" in rendered: + lines.append(f"{indent} {name}:\n") + lines.extend(_format_block(indent, rendered)) + else: + lines.append(f"{indent} {name}: {rendered}\n") return "".join(lines) diff --git a/testing/test_pluginmanager.py b/testing/test_pluginmanager.py index e36070f2..9d01b427 100644 --- a/testing/test_pluginmanager.py +++ b/testing/test_pluginmanager.py @@ -991,7 +991,7 @@ def write(message: str) -> None: assert result == "\ud800" assert out == [ - " he_method1 [hook]\n arg: \\ud800\n", + " he_method1 [hook]\n arg: '\\ud800'\n", " finish he_method1 --> \\ud800 [hook]\n", ] diff --git a/testing/test_tracer.py b/testing/test_tracer.py index a7527cab..8f350535 100644 --- a/testing/test_tracer.py +++ b/testing/test_tracer.py @@ -1,3 +1,6 @@ +import enum +import pathlib + import pytest from pluggy import HookimplMarker @@ -175,15 +178,53 @@ def __repr__(self) -> str: return "\ud800" -def test_dictargs_keep_str_rendering(rootlogger: TagTracer) -> None: - """Values keep their ``str`` rendering, the trace is a log not a repr dump.""" +def test_plain_tokens_stay_bare(rootlogger: TagTracer) -> None: + """A value that reads unambiguously as itself is not dressed up.""" out = rootlogger._format_message(["test"], ["call", {"name": "value", "n": 1}]) assert out == "call [test]\n name: value\n n: 1\n" +def test_whitespace_strings_are_quoted(rootlogger: TagTracer) -> None: + """Quotes show where a value starts and ends once it carries whitespace.""" + out = rootlogger._format_message(["test"], ["call", {"val": " padded "}]) + assert out == "call [test]\n val: ' padded '\n" + + +def test_empty_string_is_visible(rootlogger: TagTracer) -> None: + """An empty value is otherwise indistinguishable from no value at all.""" + out = rootlogger._format_message(["test"], ["call", {"left": "", "right": "x"}]) + assert out == "call [test]\n left: ''\n right: x\n" + + +def test_enum_shows_member_name(rootlogger: TagTracer) -> None: + class Exit(enum.IntEnum): + FAILED = 1 + + out = rootlogger._format_message(["test"], ["call", {"status": Exit.FAILED}]) + assert out == "call [test]\n status: \n" + + +def test_pathlike_shows_its_type(rootlogger: TagTracer) -> None: + """Two arguments printing the same path may well be different types.""" + out = rootlogger._format_message( + ["test"], ["call", {"p": pathlib.PurePosixPath("/x")}] + ) + assert out == "call [test]\n p: PurePosixPath('/x')\n" + + +def test_multiline_value_is_boxed(rootlogger: TagTracer) -> None: + """A block stays attached to its key instead of escaping to column 0.""" + out = rootlogger._format_message( + ["test"], ["call", {"expl": "first\nsecond\nthird"}] + ) + assert out == ( + "call [test]\n expl:\n | first\n | second\n \\ third\n" + ) + + def test_dictargs_escape_surrogate_values(rootlogger: TagTracer) -> None: out = rootlogger._format_message(["test"], ["test", {"arg": "\ud800"}]) - assert out == "test [test]\n arg: \\ud800\n" + assert out == "test [test]\n arg: '\\ud800'\n" out.encode() From 7741ed9c8f791bdd9f14a7cc9d6cac8cea93f2d2 Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Sat, 12 Sep 2026 22:40:56 +0200 Subject: [PATCH 2/3] Add changelog fragment for the traced value rendering Co-Authored-By: Claude Opus 5 (1M context) Co-Authored-By: Claude Code --- changelog/729.feature.rst | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/729.feature.rst diff --git a/changelog/729.feature.rst b/changelog/729.feature.rst new file mode 100644 index 00000000..842fa1e1 --- /dev/null +++ b/changelog/729.feature.rst @@ -0,0 +1,5 @@ +Traced values now gain detail where ``str()`` is ambiguous: an empty string or a string +carrying whitespace is quoted, an enum member shows its name, and a path shows its type, +so that two arguments pointing at the same place are distinguishable. Values that read +unambiguously as themselves are unchanged, and a value spanning several lines is drawn +as a block attached to its key instead of running into the surrounding trace. From 2143bb658d8dd6d2900e90737e8271ce54a7320f Mon Sep 17 00:00:00 2001 From: Ronny Pfannschmidt Date: Sat, 12 Sep 2026 22:46:07 +0200 Subject: [PATCH 3/3] Cover the repr guards added for traced values _safe_repr is only reached for enums, paths and quoted strings, which all have working reprs in the existing tests, so its guards were dead in coverage. A path-like with a broken repr is the #424 scenario applied to a value the heuristic sends through repr. _tracing.py is back at 100% statement and branch coverage. Co-Authored-By: Claude Opus 5 (1M context) Co-Authored-By: Claude Code --- testing/test_tracer.py | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/testing/test_tracer.py b/testing/test_tracer.py index 8f350535..b21789bb 100644 --- a/testing/test_tracer.py +++ b/testing/test_tracer.py @@ -1,4 +1,5 @@ import enum +import os import pathlib import pytest @@ -330,3 +331,34 @@ def __str__(self) -> str: with pytest.raises(KeyboardInterrupt): rootlogger._format_message(["test"], ["test", {"arg": Broken()}]) + + +class BrokenPath(os.PathLike[str]): + """A path-like whose repr is broken, as in #424 but for a traced path.""" + + def __fspath__(self) -> str: + raise NotImplementedError("the tracer must not resolve the path") + + def __repr__(self) -> str: + raise RuntimeError("repr is broken") + + +def test_broken_repr_on_pathlike_does_not_raise(rootlogger: TagTracer) -> None: + out = rootlogger._format_message(["test"], ["test", {"p": BrokenPath()}]) + assert "RuntimeError('repr is broken') raised in repr()" in out + assert "BrokenPath object at 0x" in out + out.encode() + + +def test_keyboard_interrupt_from_repr_propagates(rootlogger: TagTracer) -> None: + """Ctrl-C while rendering a value that goes through repr still interrupts.""" + + class Interrupting(os.PathLike[str]): + def __fspath__(self) -> str: + raise NotImplementedError("the tracer must not resolve the path") + + def __repr__(self) -> str: + raise KeyboardInterrupt + + with pytest.raises(KeyboardInterrupt): + rootlogger._format_message(["test"], ["test", {"p": Interrupting()}])