From 1d2c05735c12c9bb8d2e9e9a9a9fe9efbafa938b Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 17 Apr 2026 17:22:55 +0200 Subject: [PATCH] refactor: surface SM012 as a Diagnostic and depublicize migrations helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `check_settings_registration` now returns `list[Diagnostic]` and the boot path routes it through `print_diagnostics`, so SM012 shows up with the same framing as every other diagnostic code instead of a stray `logger.warning`. - Rename `_migrations.py` to `migrations.py` — test conftests import `resolve_head_revision` across the package boundary, so the leading underscore was misleading. The remaining `_*.py` hosting files are genuinely intra-package and stay underscored. - CLAUDE.md: `app.state._settings` was stale (the 2026-04-17 app.state reorg moved to `app.state.`). Also list SM007 and SM012 in the diagnostic-code summary. --- CLAUDE.md | 4 +-- conftest.py | 2 +- .../simple_module_hosting/_phase_helpers.py | 30 +++++++++++-------- .../simple_module_hosting/app_builder.py | 6 ++-- .../{_migrations.py => migrations.py} | 0 modules/users/tests/conftest.py | 2 +- 6 files changed, 25 insertions(+), 19 deletions(-) rename framework/hosting/simple_module_hosting/{_migrations.py => migrations.py} (100%) diff --git a/CLAUDE.md b/CLAUDE.md index eea98b7d..c735eb03 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -68,7 +68,7 @@ Standard mixins in `simple_module_db.mixins`: `AuditMixin`, `SoftDeleteMixin` (b - **SQLModel is the project-wide standard for every model** — both DB tables (`table=True`) and DTOs (plain `SQLModel` subclasses). Do not use Pydantic `BaseModel` or SQLAlchemy `DeclarativeBase` + `Mapped[...]` in module code. - **300-line cap** on `.py`/`.ts`/`.tsx` files, enforced by `scripts/check_file_size.py` in CI (exempts vendored shadcn components under `packages/ui/src/components/ui/**`). If you approach the cap, split by responsibility — don't rewrite to squeeze under. -- **Per-module settings**: env-var prefix `SM__*`, stored on `app.state._settings` inside `register_settings(app)`. `SM012` warns if `register_settings` is overridden without adding to `app.state`. +- **Per-module settings**: env-var prefix `SM__*`, stored on `app.state.` inside `register_settings(app)` as a module-owned dataclass. `SM012` warns if `register_settings` is overridden without adding `app.state.`. - **Framework vs plugin coupling**: `SM009` is an error if `framework/*` directly imports from a plugin module. Framework code must not reach into `modules/`. - **Zod schemas with translated messages** must be constructed inside a hook (`useT()`) — never at module scope, or they freeze against the first render's locale. - **Locales**: ship `/locales/.json` and declare in `ModuleBase.locale_dirs()` with the module's lowercase name as the namespace. `{"browse": {"title": "X"}}` flattens to `.browse.title`. Pluralize with CLDR suffixes (`_one`, `_other`, ...). @@ -76,7 +76,7 @@ Standard mixins in `simple_module_db.mixins`: `AuditMixin`, `SoftDeleteMixin` (b ## Diagnostic codes -Meaningful codes when reading `make doctor` output: `SM001` missing meta (error), `SM003` orphan page / `SM004` phantom render (warn), `SM008` duplicate name (error), `SM009` framework→plugin import (error), `SM010` DB revision behind head (error), `SM011` module table not in migration history (warn), `SM013`–`SM016` locale issues. In production, errors fail boot. +Meaningful codes when reading `make doctor` output: `SM001` missing meta (error), `SM003` orphan page / `SM004` phantom render (warn), `SM007` module overrides no hooks (info), `SM008` duplicate name (error), `SM009` framework→plugin import (error), `SM010` DB revision behind head (error), `SM011` module table not in migration history (warn), `SM012` `register_settings` overridden but nothing on `app.state.` (warn, fires at dev boot only), `SM013`–`SM016` locale issues. In production, errors fail boot. ## Tests & fixtures diff --git a/conftest.py b/conftest.py index 800c3e29..e90bfe75 100644 --- a/conftest.py +++ b/conftest.py @@ -69,7 +69,7 @@ def _ensure_models_imported() -> list: @lru_cache(maxsize=1) def _alembic_head() -> str | None: """Cached head revision — cannot change within a pytest run.""" - from simple_module_hosting._migrations import resolve_head_revision + from simple_module_hosting.migrations import resolve_head_revision return resolve_head_revision() diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index 14c670a9..2e8cad17 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -129,12 +129,14 @@ def mount_module_static_dirs(app: FastAPI, modules: list) -> None: ) -def check_settings_registration(app: FastAPI, modules: list) -> None: +def check_settings_registration(app: FastAPI, modules: list) -> list[Diagnostic]: """SM012: warn if a module overrides register_settings but added nothing to app.state. - New convention (2026-04-17): modules store their state at - ``app.state.`` as a module-owned dataclass. + Must run after Phase 4 (register_settings) and therefore can't join the + Phase 2 diagnostics pass; returning a list lets the caller route it through + the same ``print_diagnostics`` sink. """ + diagnostics: list[Diagnostic] = [] for mod in modules: cls = type(mod) if "register_settings" not in cls.__dict__: @@ -142,14 +144,16 @@ def check_settings_registration(app: FastAPI, modules: list) -> None: mod_prefix = mod.meta.name.lower() if hasattr(app.state, mod_prefix): continue - diag = Diagnostic( - level=DiagnosticLevel.WARNING, - code="SM012", - message="register_settings() was overridden but added nothing to app.state", - module_name=mod.meta.name, - suggestion=( - f"Store your module state on app.state " - f"(e.g., app.state.{mod_prefix} = {mod.meta.name}Services(...))" - ), + diagnostics.append( + Diagnostic( + level=DiagnosticLevel.WARNING, + code="SM012", + message="register_settings() was overridden but added nothing to app.state", + module_name=mod.meta.name, + suggestion=( + f"Store your module state on app.state " + f"(e.g., app.state.{mod_prefix} = {mod.meta.name}Services(...))" + ), + ) ) - logger.warning("%s", diag) + return diagnostics diff --git a/framework/hosting/simple_module_hosting/app_builder.py b/framework/hosting/simple_module_hosting/app_builder.py index 9af62fac..a75300ae 100644 --- a/framework/hosting/simple_module_hosting/app_builder.py +++ b/framework/hosting/simple_module_hosting/app_builder.py @@ -22,7 +22,6 @@ from simple_module_db.session import init_db from simple_module_hosting._inertia_setup import setup_inertia -from simple_module_hosting._migrations import check_migrations from simple_module_hosting._phase_helpers import ( check_settings_registration, install_middleware, @@ -31,6 +30,7 @@ ) from simple_module_hosting.health import router as health_router from simple_module_hosting.i18n_manifest import build_i18n_registry, emit_frontend_types +from simple_module_hosting.migrations import check_migrations from simple_module_hosting.settings import Settings logger = logging.getLogger(__name__) @@ -164,7 +164,9 @@ async def lifespan(app: FastAPI) -> AsyncGenerator[None, None]: mod.register_settings(app) if settings.is_development: - check_settings_registration(app, modules) + settings_diagnostics = check_settings_registration(app, modules) + if settings_diagnostics: + print_diagnostics(settings_diagnostics) # ── Phase 5: Module registrations ────────────────────── for mod in modules: diff --git a/framework/hosting/simple_module_hosting/_migrations.py b/framework/hosting/simple_module_hosting/migrations.py similarity index 100% rename from framework/hosting/simple_module_hosting/_migrations.py rename to framework/hosting/simple_module_hosting/migrations.py diff --git a/modules/users/tests/conftest.py b/modules/users/tests/conftest.py index a9a7a1ae..22c40289 100644 --- a/modules/users/tests/conftest.py +++ b/modules/users/tests/conftest.py @@ -72,7 +72,7 @@ async def _setup_app_db(application) -> None: """Create all tables and stamp alembic version so migration check passes.""" from simple_module_db.base import all_module_bases - from simple_module_hosting._migrations import resolve_head_revision + from simple_module_hosting.migrations import resolve_head_revision from sqlalchemy import text head = resolve_head_revision()