🐛 Route analyse/ logging by environment instead of a stderr handler at import#73
Merged
Merged
Conversation
…t import The analyse/ modules installed a logging.StreamHandler on their own loggers at import time, sending routine INFO progress (`Source files loaded: N`, ...) to stderr unconditionally. Wrappers that colour stderr (tox, CI log viewers) rendered these harmless lines red, and importing the package took the handler/level/stream choice away from consumers — against the stdlib logging guidance for libraries. Add an environment-aware logging facade in logger.py (`get_logger` + a swappable backend) and route the analyse/ layer through it: - default (plain library): stdlib logging with no handler installed, so INFO is dropped and WARNING+ reaches stderr via logging.lastResort. - CLI: the existing rich logger — INFO to stdout, WARNING to stderr, honouring --verbose/--quiet (now added to the `analyse` command too). - Sphinx: sphinx.util.logging — progress at VERBOSE, warnings carrying type="codelinks" + a subtype so they are suppressible and rendered on the Sphinx warning stream (mirrors sphinx-needs' logging helper). utils.py's git warnings previously used logging.warning() on the root logger (triggering basicConfig); they now go through the facade with subtypes. The dead logging block in oneline_parser.py (no log calls) is removed. Closes #72
…Runner mix_stderr) typer >= 0.24 (CI resolved 0.26.7) removed `mix_stderr` from `CliRunner`, so error output from `typer.BadParameter` now lands only on stderr and `result.stdout` is empty for error cases. The negative CLI tests asserted on `result.stdout`, so they began failing on every matrix cell once CI picked up the newer typer (independent of the logging change — they fail on main too). `result.output` carries the error text under both old and new typer, so use it for the error-path assertions. Success-output assertions (test_discover) are unaffected and left as-is.
…g test need link Routing the analyse layer's warnings through Sphinx (this PR) means they now reach the docs build, which runs `sphinx-build -nW` (warnings as errors). Two problems surfaced: - In a detached HEAD (CI checkouts), `.git/HEAD` holds the commit SHA directly. `get_current_rev` treated that as an error, warned (`codelinks.git_head`) and returned None — tripping `-W`. The SHA *is* the rev, so return it. This also yields correct remote source URLs in detached-HEAD builds. Adds a regression test for the detached-HEAD case. - The codelinks comment atop tests/test_logger.py declared need TEST_LOG_1 with a link to a non-existent IMPL_LOG_ROUTING, producing a sphinx-needs unknown-link warning. Drop the traceability comment (no matching impl need exists to link to).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #73 +/- ##
==========================================
+ Coverage 90.06% 90.79% +0.72%
==========================================
Files 29 30 +1
Lines 2628 2771 +143
Branches 306 305 -1
==========================================
+ Hits 2367 2516 +149
+ Misses 165 160 -5
+ Partials 96 95 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…down behind -v By default a Sphinx build showed nothing from codelinks (progress was emitted at VERBOSE, i.e. only with `sphinx-build -v`), and even then the lines lacked project context. Emit a per-project marker from `SourceAnalyse.run()` — used by both the CLI (`AnalyseProjects`) and the Sphinx `src-trace` directive — so it appears in both frontends: - INFO (default-visible): `codelinks [<project>]: <N> files, <M> markers` (Sphinx status stream / CLI stdout, silenced by `--quiet`). - DEBUG (only with `-v` / `--verbose`): per-type breakdown — comments, oneline-needs, id-refs, marked-rst. `SourceAnalyse` gains a `name` kwarg (the project) which both callers pass. Adds a `debug` level to the logging facade (Sphinx `verbose()`, CLI rich `debug()`), and switches the Sphinx `info` mapping from `verbose()` to `info()` so the summary is visible in a normal build. The context-less per-step logging in `analyse.py` is removed in favour of the labelled summary.
"1 file, 1 marker" instead of "1 files, 1 markers"; applies to the default summary and the -v breakdown (comments / oneline needs / id-refs / marked-rst blocks) via a small `_count` helper.
The pluralization test imported the helper inside the function; PLC0415 is only ignored for cmd.py, so the in-function import failed the ruff pre-commit hook. Move it to the top-level import.
patdhlk
approved these changes
Jun 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #72. The
analyse/layer installed alogging.StreamHandleron its own loggers at import time, so routine INFO progress (Source files loaded: N, …) went to stderr unconditionally — reddened by tox/CI even though the build succeeds (exit 0) — and importing the package took the handler/level/stream choice away from consumers (against the stdlib guidance for libraries).This adds an environment-aware logging facade and routes the shared
analyse/layer through whichever frontend is active, modelled onsphinx_needs/logging.py.What changed
logger.py— newget_logger(name)facade over a swappable backend:logging.lastResort.--verbose/--quiet.sphinx.util.logging→ progress atVERBOSE, warnings carryingtype="codelinks"+ asubtype(suppressible viasuppress_warnings, on the Sphinx warning stream).analyse/{analyse,utils,projects,oneline_parser}.py— drop the import-time handler; useget_logger(__name__).utils.py's git warnings (previouslylogging.warning()on the root logger, which triggersbasicConfig) now go through the facade with subtypes.oneline_parser.py's dead logging block (no log calls) is removed.analysecommand gains-v/--verbose&-q/--quietand callsconfigure_cli(...); the Sphinxsetup()callsconfigure_sphinx().Behaviour
lastResort)--quietcodelinks.<subtype>)-vThe accessible-pygments double-print noted in the issue disappears too, since codelinks no longer emits INFO that propagates to the root logger.
propagateis left untouched (so consumers can still capture our logs).Testing
tests/test_logger.py: no handler installed at import (regression guard for analyse/* installs a stderr logging handler at import; routine INFO progress pollutes stderr (renders red under tox/CI) #72); default mode drops INFO / emits WARNING; CLI routes INFO→stdout & WARNING→stderr with--quietsuppressing INFO; Sphinx routes viaSphinxLoggerAdapterwithtype/subtype.tests/test_cmd.pyfor theanalyseprogress /--quietgating.mypy --strictclean,ruffclean on changed files.sphinx-buildoftests/data/sphinx: the INFO progress no longer appears on stderr (was flooded before, including the doubled accessible-pygments lines); only genuine warnings remain.