Skip to content

Stop configuring logging at import time#193

Merged
gitronald merged 5 commits into
devfrom
feature/logging-no-import-side-effect
Jul 12, 2026
Merged

Stop configuring logging at import time#193
gitronald merged 5 commits into
devfrom
feature/logging-no-import-side-effect

Conversation

@gitronald

Copy link
Copy Markdown
Owner

Implements .planners/plans/057-logging-no-import-side-effect/plan.md

Removes the module-scope Logger().start(__name__) calls (ten sites) so plain import WebSearcher no longer attaches a JSONL handler to the root logger or forces root to DEBUG. Modules now use plain logging.getLogger(__name__); the package installs a NullHandler on its own logger; crawl-time configuration in SearchEngine.__init__ is untouched.

Verified:

  • import WebSearcher leaves logging.getLogger().handlers == [] (was a root StreamHandler + level DEBUG)
  • a downstream logging.basicConfig(...) after import now takes effect (was a silent no-op)
  • constructing a requests-backend SearchEngine still installs the JSONL console + file sinks and writes the crawl-log schema unchanged
  • 643 tests and 102 snapshots pass; ruff/pyrefly clean

@gitronald

Copy link
Copy Markdown
Owner Author

Code review (xhigh: 10 angles → verify → sweep)

Ten independent finder angles (line-scan, removed-behavior audit, cross-file trace, language pitfalls, logger-hierarchy correctness, reuse, simplification, efficiency, altitude, conventions), one adversarial verifier per deduped candidate, then a fresh-eyes gap sweep. Five candidates survived; verdicts and outcomes:

  1. CONFIRMED, fixed (b3fd8df)Logger.start() kept a dead, misleading default (name bound to logger.py's own __name__, so an argless call would silently yield a logger named WebSearcher.logger), and nothing warned that start() runs dictConfig and reconfigures root process-wide. name is now required and the docstring states the entry-point-only constraint.
  2. CONFIRMED, fixed (b3fd8df) — nothing pinned the PR's core invariant in CI. Added test_import_configures_no_logging: a subprocess probe (fresh interpreter — in-process assertions would be order-dependent, since other tests run dictConfig and WebSearcher is already in sys.modules) asserting root handlers stay [], root level stays WARNING, and the package NullHandler is present.
  3. CONFIRMED, fixed (b3fd8df) — the changelog entry omitted a second observable change: import no longer force-sets third-party logger levels (requests/urllib3 → WARNING, asyncio/chardet.charsetprober/parso → INFO), so an app running root at DEBUG now sees urllib3 chatter from the SearchEngine-free HTTP helpers (e.g. download_locations). Clause added.
  4. CONFIRMED behavior, conscious no-op — the NullHandler addHandler is not reload-idempotent (importlib.reload stacks handlers). Kept unguarded deliberately: requests and urllib3 ship the identical unguarded idiom (verified in this venv's installed sources), NullHandler.emit is a no-op so stacking has no functional effect, and %autoreload only re-executes __init__.py when that file itself changes.
  5. Conscious no-op — no ruff banned-api (TID251) guard confining Logger().start to searchers/. The new regression test pins the invariant in CI; a lint ban would add config surface for marginal gain over that.

Verified clean: no consumer anywhere reads the four deleted module log attributes; logger names are identical pre/post change (probed); no propagate=False orphaning; demos/CLI use print() for UX; README's logging example is crawl-path only (still accurate); no other import-time logging mutation remains in the package.

Gate after fixes: 644 tests + 102 snapshots pass, ruff + ruff-format + pyrefly clean. Gap sweep over the post-fix diff: no new findings.

@gitronald
gitronald marked this pull request as ready for review July 12, 2026 00:08
@gitronald
gitronald merged commit 0781d9d into dev Jul 12, 2026
6 checks passed
@gitronald
gitronald deleted the feature/logging-no-import-side-effect branch July 12, 2026 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant