Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions .claude/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ Rename a Python name with `just rename` rather than by substitution: it resolves

When reviewing code, whether standalone or as part of the TDD refactor step, pay attention to the following:

- **Comments**: a comment or docstring says what *this* code does as it stands, never where the increment is heading, and only what the signature and the code don't already say. Delete a sentence that narrates or justifies an edit, that says what the code is not or is unlike, or that restates a parameter, return type, or default. Delete one that describes what a caller or a collaborator does, or that names a concept from a layer further out than the module it sits in. A test's docstring answers to the same rules: it says which case the test pins, while the reason the behaviour exists belongs in the code under test and in the README. Keep a contrast only when the reader has to act on the difference. When a signature changes, the docstring usually needs a word in its summary, not a new sentence. A docstring that matches the one beside it has been copied rather than checked: read both against these rules, since a defect copied reads as a convention.
- **Comments**: a comment or docstring says what *this* code does as it stands, never where the increment is heading, and only what the signature and the code don't already say. Delete a sentence that narrates or justifies an edit, that says what the code is not or is unlike, or that restates a parameter, return type, or default. The summary already names what the function returns, so don't add a sentence saying what the return value is for. Delete one that describes what a caller or a collaborator does, or that names a concept from a layer further out than the module it sits in. A test's docstring answers to the same rules: it says which case the test pins, while the reason the behaviour exists belongs in the code under test and in the README. Keep a contrast only when the reader has to act on the difference. When a signature changes, the docstring usually needs a word in its summary, not a new sentence. A docstring that matches the one beside it has been copied rather than checked: read both against these rules, since a defect copied reads as a convention.
- **Duplication**: look for the same decision taken in more than one place, rather than for repeated lines. A rule every module has to remember to apply is duplication too: prefer stating it once, somewhere it cannot be forgotten. A helper callers have to remember to call can be forgotten as well, so prefer a check that reads the code itself. In tests, repeated setup or a repeated assertion is worth naming as a helper.
- **Reuse**: look for an existing type, test helper, or fixture before writing a new one. The misses to watch for: a fixture's value spelled out as a literal, a mixin's setup redone inline, and the same builder defined in two modules rather than in the shared one.
- **Complexity**: a function should hold one decision. Watch for nesting, for flag parameters that make one function do two things, and for long parameter lists. When a docstring needs several sentences to describe the control flow, the code is doing too much, rather than the docstring being too short.
Expand Down Expand Up @@ -50,7 +50,7 @@ A few rules that keep the cycle honest:
1. New tests follow the conventions of the nearest existing test for the same kind of behaviour, unless that test breaks a rule below. They cover the cases it covers too: a construct of an existing shape has the same edges, so read those off that test before choosing which case to start with. A table of cases loops with `subTest` naming each case, rather than repeating an assertion or a helper call, and reports one failure per failing case, so predict that many failures.
2. Build a behaviour before its off-switch: don't test an opt-out, a flag, or any other suppression until the thing it suppresses exists.
3. Assert what does and doesn't happen, rather than saying it in a docstring: an assertion is checked on every run, a docstring claim is checked by nobody. A test that asserts nothing was found passes just as well when nothing was examined, so assert that something was. A test's name is not evidence of what it guards, and neither is a green run, so settle what a guard catches with `just mutate`, a duplicate you would fold or delete included. Pick the stub from the regression that guard defends against, not from the nearest line to mutate: one that leaves the guard green says nothing about it, and one that fails a dozen others says little more, since it shows the suite reacting rather than that guard.
4. A test that pins down existing behaviour drives no code, so predict it passes and say so; it closes a gap in intent, not in behaviour.
4. A test that pins down existing behaviour drives no code, so predict it passes and say so; it closes a gap in intent, not in behaviour. A step that only removes behaviour drives no test either, so don't offer one asserting the removed thing is gone. Delete the tests that guarded it, and predict what the deletion leaves behind: the suite green, the count down by exactly the tests deleted, and a name those tests were the last outside caller of now private.
5. Coverage must stay at 100%, which `just test` already enforces, so a passing run needs no separate coverage command. A gap after implementing points at a test case worth adding, not at a line worth excluding. A step must not end with the new code uncovered: when the test you chose mocks the collaborator the new code lives on, add a second test in the same step that reaches that code. The one file the gate doesn't reach is `tools/fixit_rules.py`, which coverage omits: pin each of its branches with the VALID and INVALID cases `just fixit` runs.
6. Treat a failing existing test as a signal: work out whether its premise legitimately changed and say why, rather than patching the assertion to match the new output.
7. A green run can prove nothing, so check it ran what you think: read the tail of `just test` and `just check` yourself instead of grepping or counting their output, since a filter that matches nothing exits non-zero and silently skips the rest of an `&&` chain. A filter that prints nothing has told you nothing, so never read that silence as a pass. Compare the test count whenever imports move or test methods are renamed, since a module that fails to import, or a method whose new name collides with an existing one, drops tests without a word. The same goes for the output you quote from: never read a count off something you piped through `head`.
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

## [Unreleased]

### Changed

- Remove the column that named the Update-time module each log line came from. Closes [#230](https://github.com/ICTU/update-time/issues/230).

### Fixed

- Read a GitHub repository URL in three more spellings: git's scp-like `git@github.com:owner/repo` form, npm's `github:owner/repo` host shorthand, and npm's bare `owner/repo` shorthand. A pre-commit hook whose repository uses the scp-like form is updated instead of being silently left alone. The changelog of a package whose npm `repository` uses any of the three is found instead of being reported missing. Closes [#224](https://github.com/ICTU/update-time/issues/224).
Expand Down
92 changes: 16 additions & 76 deletions src/update_time/io/log.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,8 @@

import logging
import re
import sys
from dataclasses import dataclass
from logging import DEBUG, ERROR, INFO, WARNING
from pathlib import Path
from typing import TYPE_CHECKING

from rich.console import Console
Expand All @@ -21,7 +19,7 @@
from update_time.primitives.timestamp import days_since

if TYPE_CHECKING:
from types import FrameType
from pathlib import Path

from requests import Response
from rich.text import Text
Expand Down Expand Up @@ -117,65 +115,23 @@ def _restyle_delimited(text: Text, pattern: re.Pattern[str], style: str, *, keep

# The theme adds the styles `LogHighlighter` applies: `repr.digest` for a whole `sha256:` digest and `repr.dependency`
# (bold white) for a dependency name; a file location reuses Rich's built-in `repr.filename`, so it needs no entry.
# When colour is off, all render as plain text. The theme and formats are shared with `docs/generate_log_svg.py`,
# which logs its sample through `Logger` itself, so the README screenshot renders exactly like the real output.
# When colour is off, all render as plain text. The theme is shared with `tools/generate_log_svg.py`, which logs its
# sample through `Logger` and `configure_logging`, so the README screenshot renders exactly like the real output.
LOG_THEME = Theme({"repr.digest": "dim", "repr.dependency": "bold white"})
LOG_TIME_FORMAT = "[%X]"
LOG_MESSAGE_FORMAT = "%(message)s"
_LOG_TIME_FORMAT = "[%X]"
_LOG_MESSAGE_FORMAT = "%(message)s"


# This wrapper, and the packages that log on behalf of the updaters (see `attribute_logs_to_caller`). Frames in
# these are skipped when determining a log record's origin, so the reported origin is the updater that triggered the
# log rather than this wrapper or the shared machinery in between.
_wrapper_file = Path(__file__).resolve()
_helper_packages: set[Path] = set()


def attribute_logs_to_caller(package_file: str) -> None:
"""Register a package whose frames should be skipped when determining a log record's origin.

A package whose modules log on behalf of the updaters registers itself, passing its `__init__.py`'s `__file__`,
so the frames of every module in it are walked past and a log record is attributed to the updater that triggered
it rather than to the shared machinery in between. Registration covers the whole package, so a module added to it
needs none of its own.
"""
_helper_packages.add(Path(package_file).resolve().parent)


def _is_helper_frame(filename: str) -> bool:
"""Return whether the frame's file is this wrapper or a module in a registered package."""
path = Path(filename).resolve()
return path == _wrapper_file or any(path.is_relative_to(package) for package in _helper_packages)


def _caller_stacklevel() -> int:
"""Return the stacklevel of the first frame outside this wrapper and the registered packages.

A fixed stacklevel can't work because some log methods are called directly by an updater while others
are dispatched through a registered package (with extra comprehension frames in between), so walk
the stack to find the originating updater frame instead.
"""
level = 1 # Start at the frame that emits the record (Logger._log) and skip helper frames from there.
try:
frame: FrameType | None = sys._getframe(level) # noqa: SLF001
except ValueError: # pragma: no cover
return level
while frame is not None and _is_helper_frame(frame.f_code.co_filename):
level += 1
frame = frame.f_back
return level
def configure_logging(console: Console, level: str) -> RichHandler:
"""Send every record at the level or above to the console, and return the handler that renders it there."""
handler = RichHandler(console=console, highlighter=LogHighlighter(), show_path=False)
logging.basicConfig(level=level, datefmt=_LOG_TIME_FORMAT, format=_LOG_MESSAGE_FORMAT, handlers=[handler])
return handler


@dataclass(frozen=True)
class LogMessage:
"""A log message: the level it is logged at and the format string the arguments are interpolated into.

A message's level is a property of the message, not of the call that emits it, so the two are declared together
and `Logger._log` emits at the level the message names. The message object itself is logged, which is what the
standard library expects of a message that is not a string: it calls `str()` on it when a handler formats the
record. Passing the object rather than the format string keeps the level available to whoever holds the message,
such as the log tests and the screenshot generator.
"""
"""A log message: the level it is logged at and the format string the arguments are interpolated into."""

level: int
format: str
Expand All @@ -190,11 +146,7 @@ def __repr__(self) -> str:


def _redundant_marker(reason: str) -> str:
"""Return the warning a marker that holds nothing back is reported as, for the reason the caller gives.

Every scope reports its own reason, and the sentence they share is spelled here, so the redundant markers read
alike whichever check found one inert.
"""
"""Return the warning a marker that holds nothing back is reported as, for the reason the caller gives."""
return (
f"Redundant update-time marker %(directive)s for %(dependency)s in %(location)s: {reason}, "
"so the marker holds nothing back"
Expand All @@ -214,20 +166,12 @@ def forget_shown_changelogs(self) -> None:
self._logged_changes.clear()

def _log(self, message: LogMessage, **fields: object) -> None:
"""Emit a log record at the message's own level, attributing it to the updater that triggered it.

Every message interpolates its arguments by name (`%(location)s`), so the fields are passed as the single
mapping the standard library's `%`-formatting fills them from.
"""
self.log.log(message.level, message, self._rendered(fields), stacklevel=_caller_stacklevel())
"""Emit a log record at the message's own level."""
self.log.log(message.level, message, self._rendered(fields))

@classmethod
def _rendered(cls, fields: dict[str, object]) -> dict[str, object]:
"""Return the fields with the ones the highlighter styles wrapped in their delimiter, and the rest as they are.

Rendering them here rather than in each log method is what keeps a message's arguments plain domain values at
the call site, and leaves one place that decides what the highlighter has to style.
"""
"""Return the fields with the ones the highlighter styles wrapped in their delimiter."""
return {name: cls._render_field(name, value) for name, value in fields.items()}

@classmethod
Expand Down Expand Up @@ -887,11 +831,7 @@ def get_logger(name: str) -> Logger:
the root logger only the first time — when it has no handlers yet — instead of building a handler on every call.
"""
if not logging.getLogger().handlers:
console = Console(stderr=True, theme=LOG_THEME)
handler = RichHandler(console=console, highlighter=LogHighlighter())
logging.basicConfig(
level=LOG_LEVEL.get(), datefmt=LOG_TIME_FORMAT, format=LOG_MESSAGE_FORMAT, handlers=[handler]
)
configure_logging(Console(stderr=True, theme=LOG_THEME), LOG_LEVEL.get())
logger = Logger(name)
_LOGGERS.append(logger)
return logger
5 changes: 0 additions & 5 deletions src/update_time/references/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1 @@
"""Decide which version each pinned reference should update to, honouring its marker, and rewrite it in place."""

from update_time.io.log import attribute_logs_to_caller

# Every module in this package logs on behalf of the updaters, so records point at the updater.
attribute_logs_to_caller(__file__)
6 changes: 3 additions & 3 deletions src/update_time/references/github.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ def _sha_pinned_reference(match: re.Match[str], dependency: str) -> Reference:
return Reference(dependency, match.group("version") if current_sha else match.group("tag"), current_sha)


def latest_pin(reference: Reference, marker: Marker, location: Location, log: Logger) -> DependencyVersion | None:
def _latest_pin(reference: Reference, marker: Marker, location: Location, log: Logger) -> DependencyVersion | None:
"""Return the latest version to (re)pin the GitHub reference to, or None to leave it unchanged.

Which version to update to is `latest_version`'s decision, resolving through `sources.github`; layered on top
Expand Down Expand Up @@ -99,12 +99,12 @@ class PinUpdater:
def update_line(self, match: re.Match[str], location: Location, marker: Marker, dependency: str = "") -> str:
"""Return the line with the reference (re)pinned to the latest version, or unchanged when it stays put.

Unchanged covers each case `latest_pin` declines: an invalid current version, a marker holding the update
Unchanged covers each case `_latest_pin` declines: an invalid current version, a marker holding the update
back, no commit SHA to pin to, and a reference already pinned and up to date. The dependency comes from the
regexp's `dependency` group; a `rev:` takes it from the `repo:` above, so it names it in `dependency` instead.
"""
reference = _sha_pinned_reference(match, matched_dependency(match, dependency))
latest = latest_pin(reference, marker, location, self.logger)
latest = _latest_pin(reference, marker, location, self.logger)
if latest is None:
return match.string
return replace_match(match, self.spell(reference, latest))
2 changes: 1 addition & 1 deletion tests/update_time/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ def _expected_call(message: LogMessage, fields: dict[str, object]) -> _Call:
— a `Location` rather than its delimiter-wrapped text — and which fields carry a delimiter is asserted where
that rendering itself is tested, in the logger's unit tests.
"""
return call(message, Logger._rendered(fields), stacklevel=ANY)
return call(message, Logger._rendered(fields))

def assert_logged(self, message: LogMessage, **fields: object) -> None:
"""Assert the message was the only record logged at its level, with the given fields."""
Expand Down
Loading