From 394e01cb99b1d0f596837d749d0754776e75c5bc Mon Sep 17 00:00:00 2001 From: Aleksey Smaga Date: Thu, 30 Apr 2026 20:43:43 +0200 Subject: [PATCH] refactor(checkers): auto-discover checker modules, drop the barrel Replace the hand-curated list in src/pgsleuth/checkers/__init__.py with a pkgutil-based auto-discovery loop. Every submodule under pgsleuth.checkers (excluding base and any _-prefixed helper) is imported on package load, which runs each module's register(...) call and populates the global registry. Why: every new-checker PR was appending an import to __init__.py, guaranteeing a textual conflict between any two open checker PRs. The barrel was also the single most common new-checker footgun (documented as such in docs/writing_checkers.md): forget the import and the rule is silently invisible. Auto-discovery removes the file that has to be edited and the bug it enabled. Add tests/test_auto_discovery.py with two assertions: - registry is non-empty after import (sanity). - every non-framework module under pgsleuth.checkers has registered at least one Checker (catches the new most-common footgun: forgetting at the bottom of the module). Update docs/writing_checkers.md: drop the 'add to barrel' step, document the auto-discovery convention and the _-prefix escape hatch for non-checker helpers. After this, a new-checker PR touches only: - src/pgsleuth/checkers/.py - docs/rules/.md - tests/checkers/test_.py - README.md (one row in the Checks table) The README is now the only remaining shared file. Conflicts on the Checks table can be eliminated separately by generating it from the registry. --- docs/writing_checkers.md | 10 ++++---- src/pgsleuth/checkers/__init__.py | 37 +++++++++++++++------------- tests/test_auto_discovery.py | 40 +++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 20 deletions(-) create mode 100644 tests/test_auto_discovery.py diff --git a/docs/writing_checkers.md b/docs/writing_checkers.md index 17ecca0..2768a8d 100644 --- a/docs/writing_checkers.md +++ b/docs/writing_checkers.md @@ -87,13 +87,15 @@ Return `None` from `check_row` to skip a row that the SQL pulled in but doesn't ## Wire it up -Three things, none of them automatic: +Two things, neither of them in `src/`: -1. **Add the import** to `src/pgsleuth/checkers/__init__.py`. **The CLI imports that barrel exactly once at startup; checkers not imported there are silently invisible.** This is the single most common new-checker mistake. +1. **Add a docs page** at `docs/rules/.md` covering rationale, an example of the smell, the fix SQL, and "when to ignore." Each `Issue` carries a `docs_url` that points here automatically — no need to wire it. -2. **Add a docs page** at `docs/rules/.md` covering rationale, an example of the smell, the fix SQL, and "when to ignore." Each `Issue` carries a `docs_url` that points here automatically — no need to wire it. +2. **Add a row** to the `## Checks` table in the README, linking to the docs page. -3. **Add a row** to the `## Checks` table in the README, linking to the docs page. +The checker module itself is auto-discovered: `src/pgsleuth/checkers/__init__.py` walks the directory and imports every submodule on package import, so the `register(MyChecker)` call at the bottom of your file runs without any barrel edit. **The single thing you must remember: call `register(MyChecker)` at the bottom of the module.** Skip that and the rule won't appear in `pgsleuth list-checkers` even though the file exists. The `tests/test_auto_discovery.py` suite catches this case. + +If you add a *helper* module that isn't a checker (shared SQL, utility code), prefix the filename with `_` so auto-discovery skips it — e.g. `_partition_helpers.py`. (`base.py` is also skipped explicitly.) ## Pick the severity diff --git a/src/pgsleuth/checkers/__init__.py b/src/pgsleuth/checkers/__init__.py index d4ea1a7..9cb28ec 100644 --- a/src/pgsleuth/checkers/__init__.py +++ b/src/pgsleuth/checkers/__init__.py @@ -1,17 +1,22 @@ -"""Checker package. Importing this module loads all built-in checkers and -registers them with the global registry.""" +"""Checker package — auto-discovers every checker module on import. -from pgsleuth.checkers import ( # noqa: F401 - column_value_at_risk, - fk_type_mismatch, - json_over_jsonb, - missing_fk_index, - missing_primary_key, - not_valid_constraints, - primary_key_type, - redundant_index, - sequence_drift, - three_state_boolean, - timestamp_without_tz, - varchar_length, -) +Each checker module ends with ``register(MyChecker)``; importing the module +runs that call and adds the class to the global registry. This file walks +the package directory and imports every submodule so that registration +happens automatically. + +The upshot for contributors: adding a new checker is *one new file* in this +directory. There is no barrel to edit and no "I added the file but the +checker didn't show up" footgun. + +``base.py`` and any ``_``-prefixed module are skipped — they're framework +or shared-helper code, not checkers. +""" + +import importlib +import pkgutil + +for _info in pkgutil.iter_modules(__path__): + if _info.name == "base" or _info.name.startswith("_"): + continue + importlib.import_module(f"{__name__}.{_info.name}") diff --git a/tests/test_auto_discovery.py b/tests/test_auto_discovery.py new file mode 100644 index 0000000..78ebc21 --- /dev/null +++ b/tests/test_auto_discovery.py @@ -0,0 +1,40 @@ +"""Verify pgsleuth.checkers auto-discovery wires every checker module. + +These tests run without a database — they only inspect the import graph +and the in-memory registry. +""" + +from __future__ import annotations + +import pkgutil + +import pgsleuth.checkers # noqa: F401 — triggers auto-discovery +from pgsleuth.checkers.base import registry + + +def _checker_module_names() -> set[str]: + """Module names that should each register at least one Checker.""" + return { + f"pgsleuth.checkers.{info.name}" + for info in pkgutil.iter_modules(pgsleuth.checkers.__path__) + if info.name != "base" and not info.name.startswith("_") + } + + +def test_registry_is_populated(): + """Sanity: importing pgsleuth.checkers yields a non-empty registry.""" + assert len(registry.names()) > 0 + + +def test_every_checker_module_registers_at_least_one_checker(): + """Each non-framework module under pgsleuth.checkers must call + register(MyChecker) on import. If a module is silently not registering, + the rule is invisible to the CLI even though the file exists.""" + expected = _checker_module_names() + registered = {cls.__module__ for cls in registry.all()} + + missing = expected - registered + assert not missing, ( + f"these checker modules did not register a Checker — " + f"forgot `register(MyChecker)` at the bottom? {sorted(missing)}" + )