From 7d34943a0fcfb5bad09cbfbc2d8659477a558a05 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 16:18:36 +0200 Subject: [PATCH 1/2] feat(rules): bootstrap 3 Python doc families (architecture, ioc, logging) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit First Python-side bootstrap PR. Same multi-doc shape as PRs #17, #18. 3 Python docs (~500-820 lines), 6 rules, single bot review cycle. Expands rule coverage from Go-only to Python. Rules added (rules/index.json: 68 -> 74): python-architecture/* (owner: python-architecture-assistant) - constructor-injection-only (MUST) — deps via __init__; methods take only runtime data. Mixing the two breaks the dep-graph visibility that makes Python's typing useful. - main-py-composition-root (SHOULD) — wire all deps in main(), never at module scope. Module-level instantiation runs at import time, making tests order-dependent. python-ioc/* (owner: python-architecture-assistant) - protocol-not-abc-for-dependencies (MUST) — typing.Protocol for dependency interfaces; abc.ABC only when concrete impls share code via super(). Protocol is the right primitive for pure contracts; ABC is the right primitive for shared inheritance. - dependencies-as-private-fields (MUST) — store injected deps on self._foo (private), never self.foo (public). Public attribute storage invites external mutation that breaks the constructor- injection immutability contract. python-logging/* (owner: python-quality-assistant) - configure-once-in-main (MUST) — logging.basicConfig only at the application entry point. Libraries call logging.getLogger(__name__) and emit; they never configure. Library-side basicConfig produces three failure modes (first-import-wins, duplicate handlers, lost log-level control). - lazy-evaluation-for-debug (MUST) — use %s placeholders for DEBUG messages with expensive interpolated values, not f-strings. F-string interpolation runs every call even when DEBUG is filtered out; %s defers evaluation to the logging library. CLAUDE.md doc-agent table updated with all 3 new mappings. Generic examples throughout (User, Order, UserService, UserRepository). No personal vault paths, no trading-domain terms. Pre-emptive grep clean. make build-index regenerated; check-index passes. --- CLAUDE.md | 3 + docs/python-architecture-patterns.md | 83 ++++++++++++++++++++++++++++ docs/python-ioc-guide.md | 33 +++++++++++ docs/python-logging-guide.md | 66 ++++++++++++++++++++++ rules/index.json | 54 ++++++++++++++++++ 5 files changed, 239 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 6522bda..206ca28 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,6 +39,9 @@ Each enforceable guide in `docs/` should have a matching agent in `agents/`. The | `go-licensing-guide.md` | `license-assistant` | | `agent-command-development-guide.md` | `agent-auditor` + `slash-command-auditor` | | `claude-code-skill-writing-guide.md` | `skill-auditor` | +| `python-architecture-patterns.md` | `python-architecture-assistant` | +| `python-ioc-guide.md` | `python-architecture-assistant` | +| `python-logging-guide.md` | `python-quality-assistant` | Reference-only docs (patterns, setup guides) don't need agents. diff --git a/docs/python-architecture-patterns.md b/docs/python-architecture-patterns.md index 4755386..83560b6 100644 --- a/docs/python-architecture-patterns.md +++ b/docs/python-architecture-patterns.md @@ -12,6 +12,49 @@ The standard pattern for Python services follows this structure: ## 1. Core Pattern: Constructor Injection +### RULE python-architecture/constructor-injection-only (MUST) + +**Owner**: python-architecture-assistant +**Applies when**: a Python service class receives a dependency (logger, repository, API client, validator, etc.) through a method parameter, a global module variable, or `self.foo = bar.foo` mutation after construction — instead of through `__init__`. +**Enforcement**: judgment (semantic — requires distinguishing dependency injection from runtime parameter passing) +**Why**: Mixing dependency injection with runtime parameter passing breaks the contract Python's type system relies on. Constructor injection makes the dep set visible at `__init__`, immutable after construction, and self-documenting at the type signature. Method-level injection means tests have to construct the full dep graph for every call site; global injection means tests are order-dependent. Methods should receive only the data needed to do their job — `create_user(user: User)`, not `create_user(repo, logger, validator, user)`. + +#### Bad + +```python +class UserService: + def create_user( + self, + repo: UserRepository, # ← dependency, not runtime data + logger: Logger, # ← dependency, not runtime data + validator: UserValidator, # ← dependency, not runtime data + user: User, # ← actual runtime data + ) -> None: + validator.validate(user) + repo.save(user) + logger.info(f"Created user {user.id}") +``` + +#### Good + +```python +class UserService: + def __init__( + self, + repo: UserRepository, + logger: Logger, + validator: UserValidator, + ): + self._repo = repo + self._logger = logger + self._validator = validator + + def create_user(self, user: User) -> None: # only runtime data + self._validator.validate(user) + self._repo.save(user) + self._logger.info(f"Created user {user.id}") +``` + ### Protocol Definition ```python @@ -50,6 +93,46 @@ class UserService: ## 2. main.py Pattern (Composition Root) +### RULE python-architecture/main-py-composition-root (SHOULD) + +**Owner**: python-architecture-assistant +**Applies when**: a Python application instantiates service objects at module level (top of `repository.py`, `handler.py`, etc.) instead of wiring them inside `main()` / `main.py`. +**Enforcement**: judgment (file-scope check: module-level `Service()` / `Repository()` calls outside `if __name__ == "__main__":` blocks) +**Why**: Module-level instantiation runs at *import time* — order-dependent, hard to test, impossible to mock for unit tests. The composition root pattern says: configuration parsing, infrastructure setup, and service wiring all happen in one place (`main.py` → `main(argv)`), in a deterministic order, with explicit dependency edges. Tests can then construct their own composition root with mock infrastructure; production constructs the real one. Module-level state collapses this clean separation into spaghetti. + +#### Bad + +```python +# user_service.py +from infrastructure import database, api_client # imports trigger connection! + +repo = SqlUserRepository(database) # module-level — runs at import +service = UserService(repo, api_client) # module-level — runs at import +``` + +#### Good + +```python +# main.py — composition root +def main(argv): + logging.basicConfig(...) + db_url = os.getenv("DATABASE_URL") + api_key = os.getenv("API_KEY") + + database = Database(db_url) # infrastructure first + api_client = ApiClient(api_key) + + repo = SqlUserRepository(database) # services composed explicitly + service = UserService(repo, api_client) + + handler = UserHandler(service) + HttpServer(8080, handler).run() + +# user_service.py — no module-level instantiation +class UserService: + def __init__(self, repo: UserRepository, api: ApiClient): ... +``` + The `main.py` is the **composition root** where all dependencies are wired together. ### Structure diff --git a/docs/python-ioc-guide.md b/docs/python-ioc-guide.md index b7355e8..4476955 100644 --- a/docs/python-ioc-guide.md +++ b/docs/python-ioc-guide.md @@ -53,6 +53,13 @@ class UserValidator(Protocol): ## Rules +### RULE python-ioc/protocol-not-abc-for-dependencies (MUST) + +**Owner**: python-architecture-assistant +**Applies when**: a Python service interface (dependency surface, e.g. `UserRepository`, `Logger`) is declared with `abc.ABC` + `@abstractmethod` decorators instead of `typing.Protocol`, AND no shared implementation across concrete types motivates the ABC. +**Enforcement**: judgment (ast-grep partial: detect `class X(ABC):` declarations that only contain `@abstractmethod` methods — but the "no shared impl needed" trigger is semantic) +**Why**: Protocol gives you structural typing — any class with the right method shape satisfies it, without inheritance. That's the right primitive for dependency interfaces: mocks don't need to inherit from anything, tests don't fake an `ABC` hierarchy, and you avoid the "every protocol class has both an ABC and a Protocol declaration" duplication that ABC-first projects accrete. ABC is the right primitive when concrete implementations share code via `super()` — that's the actual reason ABCs exist. For dependency interfaces (which are pure contracts), Protocol is shorter, lighter, and friendlier to mocks. + ### Use Protocol for Dependency Interfaces **Constraint:** MUST use `Protocol` for defining dependency interfaces unless shared implementation is required. @@ -120,6 +127,32 @@ class UserService: self._validator = UserValidator() ``` +### RULE python-ioc/dependencies-as-private-fields (MUST) + +**Owner**: python-architecture-assistant +**Applies when**: a Python service class stores an injected dependency on `self.` (public attribute) instead of `self._` (single-underscore private convention). +**Enforcement**: judgment (ast-grep follow-up: `assignment` inside `__init__` with left-hand side `self.X` where `X` does not start with `_` and right-hand side is a parameter typed as a `Protocol` / `ABC` interface) +**Why**: Public attribute storage invites external mutation — `user_service.repo = MockRepository()` — which breaks the immutability contract that makes constructor injection safe in the first place. Tests that mutate inject-time fields hide behavior that production never exercises. Single-underscore prefix is Python's universal "internal, don't touch" convention; following it forces test code to compose deps the same way production does (through the constructor) and keeps the dep set observable only at `__init__`. + +#### Bad + +```python +class UserService: + def __init__(self, repo: UserRepository): + self.repo = repo # public — external code can rebind it + +# elsewhere: +user_service.repo = SomeOtherRepo() # silent contract violation +``` + +#### Good + +```python +class UserService: + def __init__(self, repo: UserRepository): + self._repo = repo # private — single-underscore convention +``` + ### Store Dependencies as Private Fields **Constraint:** MUST store injected dependencies as private fields with underscore prefix (`self._repo`). diff --git a/docs/python-logging-guide.md b/docs/python-logging-guide.md index 5a348e2..b59fb31 100644 --- a/docs/python-logging-guide.md +++ b/docs/python-logging-guide.md @@ -38,6 +38,46 @@ logger = logging.getLogger(__name__) ## Configuration Rules +### RULE python-logging/configure-once-in-main (MUST) + +**Owner**: python-quality-assistant +**Applies when**: a Python file outside the application entry point (i.e. NOT `main.py` / `__main__.py` / `cli.py` invoked as the entry) calls `logging.basicConfig()` or adds handlers to the root logger. +**Enforcement**: judgment (ast-grep follow-up: `call_expression` matching `logging.basicConfig` outside files identified as entry points by `if __name__ == "__main__":` presence; also flag `root_logger.addHandler` in library code) +**Why**: `basicConfig` is a one-shot global root-logger setup. Calling it from library code produces three failure modes: (1) first import wins, so the library's config silently overrides the application's choice depending on import order; (2) repeated calls add duplicate handlers, doubling every log line; (3) applications can't change log level without code edits in libraries they don't control. Libraries call `logging.getLogger(__name__)` and emit; configuration is the application's responsibility, and the application does it exactly once. + +#### Bad + +```python +# mylib/service.py — library configures logging — wrong +import logging + +logging.basicConfig(level=logging.INFO) # first import wins, overrides app +logger = logging.getLogger(__name__) + +class UserService: ... +``` + +#### Good + +```python +# main.py — application entry point, configure once +import logging + +logging.basicConfig( + format='%(asctime)s %(levelname)-8s [%(name)s:%(lineno)d] %(message)s', + level=logging.INFO, +) + +# mylib/service.py — library only gets a logger, never configures +import logging + +logger = logging.getLogger(__name__) + +class UserService: + def process(self): + logger.info("processing user") +``` + ### Configure Logging Once at Application Entry Point **Constraint:** Application code MUST call `logging.basicConfig()` exactly once at startup in `main.py`. Library code MUST NOT call `basicConfig()` or configure handlers. @@ -377,6 +417,32 @@ logger.warning("Failed to connect", connection_error) # Only logs first arg logger.warning("Failed to connect: %s", connection_error) ``` +### RULE python-logging/lazy-evaluation-for-debug (MUST) + +**Owner**: python-quality-assistant +**Applies when**: a Python `logger.debug(...)` / `logger.log(logging.DEBUG, ...)` call passes an f-string whose interpolation calls an expensive function (serializer, JSON dump, network fetch, large-collection traversal) instead of using `%s` placeholders with deferred arguments. +**Enforcement**: judgment (ast-grep partial: `call_expression` named `logger.debug` with an `interpreted_string_literal` first argument containing `f"..."` patterns — the "expensive operation" judgment is semantic) +**Why**: F-strings interpolate immediately at function-call time. With `logger.debug(f"...{expensive(x)}...")`, `expensive(x)` runs every call, even when DEBUG is filtered out and the message is discarded. `logger.debug("... %s ...", expensive(x))` defers `expensive(x)` to the logging library, which only evaluates arguments when the level is enabled. In hot paths with DEBUG-by-default-off production, the difference between "free" and "this expensive call runs millions of times for log lines no one sees" is measurable. The `isEnabledFor(logging.DEBUG)` guard is the explicit alternative; `%s` is the implicit-defer pattern that scales. + +#### Bad + +```python +# F-string forces expensive_serialization to run on every call, +# even when DEBUG is filtered out and the message is discarded +logger.debug(f"Details: {expensive_serialization(large_object)}") +``` + +#### Good + +```python +# %s placeholder defers evaluation to the logging library — runs only if DEBUG enabled +logger.debug("Details: %s", expensive_serialization(large_object)) + +# Explicit guard — same effect, more readable when you also need f-string features +if logger.isEnabledFor(logging.DEBUG): + logger.debug(f"Details: {expensive_serialization(large_object)}") +``` + ### Use Lazy Evaluation for Expensive DEBUG Operations **Constraint:** Code MUST use `%s` formatting (not f-strings) for DEBUG messages with expensive operations. diff --git a/rules/index.json b/rules/index.json index 069de26..f5b2807 100644 --- a/rules/index.json +++ b/rules/index.json @@ -610,5 +610,59 @@ "id": "go-time/no-time-time-in-fields", "level": "MUST", "owner": "go-time-assistant" + }, + { + "anchor": "python-architecture/constructor-injection-only", + "applies_when": "a Python service class receives a dependency (logger, repository, API client, validator, etc.) through a method parameter, a global module variable, or `self.foo = bar.foo` mutation after construction — instead of through `__init__`.", + "doc_path": "docs/python-architecture-patterns.md", + "enforcement": "judgment (semantic — requires distinguishing dependency injection from runtime parameter passing)", + "id": "python-architecture/constructor-injection-only", + "level": "MUST", + "owner": "python-architecture-assistant" + }, + { + "anchor": "python-architecture/main-py-composition-root", + "applies_when": "a Python application instantiates service objects at module level (top of `repository.py`, `handler.py`, etc.) instead of wiring them inside `main()` / `main.py`.", + "doc_path": "docs/python-architecture-patterns.md", + "enforcement": "judgment (file-scope check: module-level `Service()` / `Repository()` calls outside `if __name__ == \"__main__\":` blocks)", + "id": "python-architecture/main-py-composition-root", + "level": "SHOULD", + "owner": "python-architecture-assistant" + }, + { + "anchor": "python-ioc/dependencies-as-private-fields", + "applies_when": "a Python service class stores an injected dependency on `self.` (public attribute) instead of `self._` (single-underscore private convention).", + "doc_path": "docs/python-ioc-guide.md", + "enforcement": "judgment (ast-grep follow-up: `assignment` inside `__init__` with left-hand side `self.X` where `X` does not start with `_` and right-hand side is a parameter typed as a `Protocol` / `ABC` interface)", + "id": "python-ioc/dependencies-as-private-fields", + "level": "MUST", + "owner": "python-architecture-assistant" + }, + { + "anchor": "python-ioc/protocol-not-abc-for-dependencies", + "applies_when": "a Python service interface (dependency surface, e.g. `UserRepository`, `Logger`) is declared with `abc.ABC` + `@abstractmethod` decorators instead of `typing.Protocol`, AND no shared implementation across concrete types motivates the ABC.", + "doc_path": "docs/python-ioc-guide.md", + "enforcement": "judgment (ast-grep partial: detect `class X(ABC):` declarations that only contain `@abstractmethod` methods — but the \"no shared impl needed\" trigger is semantic)", + "id": "python-ioc/protocol-not-abc-for-dependencies", + "level": "MUST", + "owner": "python-architecture-assistant" + }, + { + "anchor": "python-logging/configure-once-in-main", + "applies_when": "a Python file outside the application entry point (i.e. NOT `main.py` / `__main__.py` / `cli.py` invoked as the entry) calls `logging.basicConfig()` or adds handlers to the root logger.", + "doc_path": "docs/python-logging-guide.md", + "enforcement": "judgment (ast-grep follow-up: `call_expression` matching `logging.basicConfig` outside files identified as entry points by `if __name__ == \"__main__\":` presence; also flag `root_logger.addHandler` in library code)", + "id": "python-logging/configure-once-in-main", + "level": "MUST", + "owner": "python-quality-assistant" + }, + { + "anchor": "python-logging/lazy-evaluation-for-debug", + "applies_when": "a Python `logger.debug(...)` / `logger.log(logging.DEBUG, ...)` call passes an f-string whose interpolation calls an expensive function (serializer, JSON dump, network fetch, large-collection traversal) instead of using `%s` placeholders with deferred arguments.", + "doc_path": "docs/python-logging-guide.md", + "enforcement": "judgment (ast-grep partial: `call_expression` named `logger.debug` with an `interpreted_string_literal` first argument containing `f\"...\"` patterns — the \"expensive operation\" judgment is semantic)", + "id": "python-logging/lazy-evaluation-for-debug", + "level": "MUST", + "owner": "python-quality-assistant" } ] From 70edb553bdb505e221a9eb29f5d8714dca25b9d9 Mon Sep 17 00:00:00 2001 From: Benjamin Borbe Date: Tue, 2 Jun 2026 16:35:17 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(rules):=20address=20PR=20#19=20review?= =?UTF-8?q?=20=E2=80=94=201=20CRITICAL=20+=207=20MAJOR=20+=203=20NIT?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot's deepest review of the session. Many real issues, mostly traceable to my Edits inserting RULE blocks ABOVE existing constraint sections instead of replacing them — produced duplicate free-form sections under each new rule. Cleaned up. CRITICAL: - configure-once-in-main would mechanically flag the doc's own logging_setup.py extraction pattern (basicConfig in a helper module wired from main.py). Refined applies_when to exempt application- private configuration helpers; enforcement now notes the helper-module exemption case. MAJOR — redundancy cleanup (4 of 7): - python-ioc: removed duplicate 'Use Protocol for Dependency Interfaces' free-form section (RULE block above covers same Bad/Good) - python-ioc: removed duplicate 'Store Dependencies as Private Fields' free-form section - python-architecture: replaced duplicate 'Protocol Definition' + 'Service Implementation' sections with a Key-points cross-reference list pointing at the canonical RULEs in python-ioc-guide.md - python-logging: removed duplicate 'Configure Logging Once at Entry Point' free-form section - python-logging: removed duplicate 'Use Lazy Evaluation for Expensive DEBUG Operations' free-form section MAJOR — scope/enforcement refinements (3 of 7): - constructor-injection-only applies_when narrowed to method-parameter DI as the primary trigger; global-var and post-construction-mutation noted as related anti-patterns covered by sibling rules (main-py-composition-root + dependencies-as-private-fields). - dependencies-as-private-fields enforcement reframed: ast-grep is a first-pass overinclusive filter (any non-_ self.X in __init__); type-annotation resolution acknowledged as impossible in ast-grep, agent makes final call. - lazy-evaluation-for-debug enforcement reframed similarly: ast-grep flags all f-strings in logger.debug as first-pass; agent decides expensive-vs-trivial after the filter. Also fixed configure-once-in-main Good example to include 'if __name__ == "__main__":' guard for consistency with the applies_when's entry-point definition. NITs (skipped — not blocking, can be addressed in future doc polish): - main-py-composition-root SHOULD vs universal-required framing - Bad example comment 'imports trigger connection!' conflates concerns - lazy-evaluation-for-debug doesn't cover non-f-string non-lazy cases make build-index regenerated; check-index passes. --- docs/python-architecture-patterns.md | 42 +++------------ docs/python-ioc-guide.md | 45 +++++----------- docs/python-logging-guide.md | 80 +++++----------------------- rules/index.json | 12 ++--- 4 files changed, 36 insertions(+), 143 deletions(-) diff --git a/docs/python-architecture-patterns.md b/docs/python-architecture-patterns.md index 83560b6..989ab1e 100644 --- a/docs/python-architecture-patterns.md +++ b/docs/python-architecture-patterns.md @@ -15,8 +15,8 @@ The standard pattern for Python services follows this structure: ### RULE python-architecture/constructor-injection-only (MUST) **Owner**: python-architecture-assistant -**Applies when**: a Python service class receives a dependency (logger, repository, API client, validator, etc.) through a method parameter, a global module variable, or `self.foo = bar.foo` mutation after construction — instead of through `__init__`. -**Enforcement**: judgment (semantic — requires distinguishing dependency injection from runtime parameter passing) +**Applies when**: a Python service class receives a dependency (logger, repository, API client, validator, etc.) through a *method parameter* instead of through `__init__`. Related but distinct anti-patterns covered by other rules: dependency from a global module variable (see `python-architecture/main-py-composition-root`); post-construction `self.foo = bar.foo` mutation (see `python-ioc/dependencies-as-private-fields`, which mandates private-field storage that makes mutation socially difficult). +**Enforcement**: judgment (semantic — requires distinguishing dependency injection from runtime parameter passing. The other two related anti-patterns have their own rules and enforcement paths.) **Why**: Mixing dependency injection with runtime parameter passing breaks the contract Python's type system relies on. Constructor injection makes the dep set visible at `__init__`, immutable after construction, and self-documenting at the type signature. Method-level injection means tests have to construct the full dep graph for every call site; global injection means tests are order-dependent. Methods should receive only the data needed to do their job — `create_user(user: User)`, not `create_user(repo, logger, validator, user)`. #### Bad @@ -55,41 +55,11 @@ class UserService: self._logger.info(f"Created user {user.id}") ``` -### Protocol Definition +**Key points** (the RULE above shows the canonical Bad/Good shape; below are the orthogonal supporting conventions): -```python -from typing import Protocol - -class UserRepository(Protocol): - def save(self, user: User) -> None: ... - def find_by_id(self, user_id: int) -> User | None: ... -``` - -### Service Implementation - -```python -class UserService: - def __init__( - self, - repo: UserRepository, - logger: Logger, - validator: UserValidator, - ): - self._repo = repo # Static dependency - self._logger = logger # Static dependency - self._validator = validator # Static dependency - - def create_user(self, user: User) -> None: # Runtime param only - self._validator.validate(user) - self._repo.save(user) - self._logger.info(f"Created user {user.id}") -``` - -**Key points:** -- Constructor receives ALL dependencies (static, mockable) -- Methods receive ONLY runtime data (request params, user input) -- Dependencies stored as private fields (`self._dep`) -- Clean separation: deps bound once, methods reusable +- Constructor receives ALL dependencies (static, mockable); methods receive ONLY runtime data. +- Dependencies stored as private fields (`self._dep`) — see [`python-ioc/dependencies-as-private-fields`](python-ioc-guide.md). +- Dependency interfaces use `Protocol` — see [`python-ioc/protocol-not-abc-for-dependencies`](python-ioc-guide.md). ## 2. main.py Pattern (Composition Root) diff --git a/docs/python-ioc-guide.md b/docs/python-ioc-guide.md index 4476955..ed5a3c7 100644 --- a/docs/python-ioc-guide.md +++ b/docs/python-ioc-guide.md @@ -60,28 +60,26 @@ class UserValidator(Protocol): **Enforcement**: judgment (ast-grep partial: detect `class X(ABC):` declarations that only contain `@abstractmethod` methods — but the "no shared impl needed" trigger is semantic) **Why**: Protocol gives you structural typing — any class with the right method shape satisfies it, without inheritance. That's the right primitive for dependency interfaces: mocks don't need to inherit from anything, tests don't fake an `ABC` hierarchy, and you avoid the "every protocol class has both an ABC and a Protocol declaration" duplication that ABC-first projects accrete. ABC is the right primitive when concrete implementations share code via `super()` — that's the actual reason ABCs exist. For dependency interfaces (which are pure contracts), Protocol is shorter, lighter, and friendlier to mocks. -### Use Protocol for Dependency Interfaces +#### Bad -**Constraint:** MUST use `Protocol` for defining dependency interfaces unless shared implementation is required. +```python +# Using ABC when no shared implementation needed +from abc import ABC, abstractmethod -**Rationale:** Protocol enables structural typing and works seamlessly with mocks without inheritance overhead. +class UserRepository(ABC): + @abstractmethod + def save(self, user: User) -> None: + pass +``` + +#### Good -**Examples:** ```python -# [GOOD] from typing import Protocol class UserRepository(Protocol): def save(self, user: User) -> None: ... def find_by_id(self, user_id: int) -> User | None: ... - -# [BAD] - Using ABC when no shared implementation needed -from abc import ABC, abstractmethod - -class UserRepository(ABC): - @abstractmethod - def save(self, user: User) -> None: - pass ``` **Protocol vs ABC Decision:** @@ -131,7 +129,7 @@ class UserService: **Owner**: python-architecture-assistant **Applies when**: a Python service class stores an injected dependency on `self.` (public attribute) instead of `self._` (single-underscore private convention). -**Enforcement**: judgment (ast-grep follow-up: `assignment` inside `__init__` with left-hand side `self.X` where `X` does not start with `_` and right-hand side is a parameter typed as a `Protocol` / `ABC` interface) +**Enforcement**: judgment (ast-grep first-pass filter: any `assignment` inside `__init__` with left-hand side `self.X` where `X` does not start with `_`. Type-annotation resolution to distinguish service deps from public data fields cannot be done in ast-grep — the agent makes the final call. Overinclusive first-pass is acceptable: most public `self.X` assignments in service classes ARE the smell.) **Why**: Public attribute storage invites external mutation — `user_service.repo = MockRepository()` — which breaks the immutability contract that makes constructor injection safe in the first place. Tests that mutate inject-time fields hide behavior that production never exercises. Single-underscore prefix is Python's universal "internal, don't touch" convention; following it forces test code to compose deps the same way production does (through the constructor) and keeps the dep set observable only at `__init__`. #### Bad @@ -153,25 +151,6 @@ class UserService: self._repo = repo # private — single-underscore convention ``` -### Store Dependencies as Private Fields - -**Constraint:** MUST store injected dependencies as private fields with underscore prefix (`self._repo`). - -**Rationale:** Prevents external mutation and clearly indicates internal implementation details. - -**Examples:** -```python -# [GOOD] -class UserService: - def __init__(self, repo: UserRepository): - self._repo = repo # Private field - -# [BAD] -class UserService: - def __init__(self, repo: UserRepository): - self.repo = repo # Public field allows external mutation -``` - ### Never Return Values from Constructors **Constraint:** MUST NOT return values from `__init__` methods. diff --git a/docs/python-logging-guide.md b/docs/python-logging-guide.md index b59fb31..5a0696c 100644 --- a/docs/python-logging-guide.md +++ b/docs/python-logging-guide.md @@ -41,8 +41,8 @@ logger = logging.getLogger(__name__) ### RULE python-logging/configure-once-in-main (MUST) **Owner**: python-quality-assistant -**Applies when**: a Python file outside the application entry point (i.e. NOT `main.py` / `__main__.py` / `cli.py` invoked as the entry) calls `logging.basicConfig()` or adds handlers to the root logger. -**Enforcement**: judgment (ast-grep follow-up: `call_expression` matching `logging.basicConfig` outside files identified as entry points by `if __name__ == "__main__":` presence; also flag `root_logger.addHandler` in library code) +**Applies when**: a Python file in *library code* (modules imported by application code, intended for reuse across applications) calls `logging.basicConfig()` or adds handlers to the root logger. **Exempt**: the application entry point (`main.py` / `__main__.py` / `cli.py`) AND application-private helper modules wired from the entry point exclusively for configuration (e.g. `logging_setup.py` — see the next section), since those are part of the entry-point configuration surface, not library code. +**Enforcement**: judgment (semantic — distinguishing library code from application-private configuration helpers requires reading the import graph; ast-grep can flag every non-entry-point `logging.basicConfig` call as a first-pass filter, but the agent must rule out the helper-module exemption case) **Why**: `basicConfig` is a one-shot global root-logger setup. Calling it from library code produces three failure modes: (1) first import wins, so the library's config silently overrides the application's choice depending on import order; (2) repeated calls add duplicate handlers, doubling every log line; (3) applications can't change log level without code edits in libraries they don't control. Libraries call `logging.getLogger(__name__)` and emit; configuration is the application's responsibility, and the application does it exactly once. #### Bad @@ -63,10 +63,15 @@ class UserService: ... # main.py — application entry point, configure once import logging -logging.basicConfig( - format='%(asctime)s %(levelname)-8s [%(name)s:%(lineno)d] %(message)s', - level=logging.INFO, -) +def main(): + logging.basicConfig( + format='%(asctime)s %(levelname)-8s [%(name)s:%(lineno)d] %(message)s', + level=logging.INFO, + ) + # ... rest of app wiring ... + +if __name__ == "__main__": + main() # mylib/service.py — library only gets a logger, never configures import logging @@ -78,46 +83,6 @@ class UserService: logger.info("processing user") ``` -### Configure Logging Once at Application Entry Point - -**Constraint:** Application code MUST call `logging.basicConfig()` exactly once at startup in `main.py`. Library code MUST NOT call `basicConfig()` or configure handlers. - -**Rationale:** Multiple configurations cause conflicts, overwrites, and duplicate log entries across modules. - -**Examples:** -```python -# [GOOD] Application entry point -# main.py -import logging - -logging.basicConfig( - format='%(asctime)s %(levelname)-8s [%(name)s:%(lineno)d] %(message)s', - level=logging.INFO, - datefmt='%Y-%m-%d %H:%M:%S' -) - -logger = logging.getLogger(__name__) -logger.info("Application started") - -# [GOOD] Library module -# mylib/service.py -import logging - -logger = logging.getLogger(__name__) # Get logger only - -class UserService: - def process(self): - logger.info("Processing user") - -# [BAD] Library configuring logging -# mylib/service.py -logging.basicConfig(level=logging.INFO) # Never do this in libraries - -# [BAD] Multiple configurations -# orders/handler.py -logging.basicConfig(level=logging.DEBUG) # Conflicts with main config -``` - ### Extract Logging Configuration to Dedicated Module **Constraint:** Applications with complex logging needs (multiple handlers, conditional configuration) SHOULD extract logging setup to dedicated `logging_setup.py` module. @@ -421,7 +386,7 @@ logger.warning("Failed to connect: %s", connection_error) **Owner**: python-quality-assistant **Applies when**: a Python `logger.debug(...)` / `logger.log(logging.DEBUG, ...)` call passes an f-string whose interpolation calls an expensive function (serializer, JSON dump, network fetch, large-collection traversal) instead of using `%s` placeholders with deferred arguments. -**Enforcement**: judgment (ast-grep partial: `call_expression` named `logger.debug` with an `interpreted_string_literal` first argument containing `f"..."` patterns — the "expensive operation" judgment is semantic) +**Enforcement**: judgment (ast-grep is a first-pass filter only: `call_expression` named `logger.debug` with an `interpreted_string_literal` first argument matching `f"..."`. Trivial f-string interpolations like `logger.debug(f"count={count}")` are NOT violations — the rule targets *expensive* interpolations (serializers, JSON dumps, network fetches, large-collection traversals). The agent makes the expensive-vs-trivial judgment after the mechanical filter.) **Why**: F-strings interpolate immediately at function-call time. With `logger.debug(f"...{expensive(x)}...")`, `expensive(x)` runs every call, even when DEBUG is filtered out and the message is discarded. `logger.debug("... %s ...", expensive(x))` defers `expensive(x)` to the logging library, which only evaluates arguments when the level is enabled. In hot paths with DEBUG-by-default-off production, the difference between "free" and "this expensive call runs millions of times for log lines no one sees" is measurable. The `isEnabledFor(logging.DEBUG)` guard is the explicit alternative; `%s` is the implicit-defer pattern that scales. #### Bad @@ -443,27 +408,6 @@ if logger.isEnabledFor(logging.DEBUG): logger.debug(f"Details: {expensive_serialization(large_object)}") ``` -### Use Lazy Evaluation for Expensive DEBUG Operations - -**Constraint:** Code MUST use `%s` formatting (not f-strings) for DEBUG messages with expensive operations. - -**Rationale:** `%s` formatting only evaluates arguments if DEBUG level is enabled, avoiding unnecessary computation. - -**Examples:** -```python -# [BAD] F-string with expensive DEBUG operation -logger.debug(f"Details: {expensive_serialization(large_object)}") -# Always evaluates, even when DEBUG disabled - -# [GOOD] Lazy evaluation with %s -logger.debug("Details: %s", expensive_serialization(large_object)) -# Only evaluates if DEBUG enabled - -# [GOOD] Explicit guard for expensive operations -if logger.isEnabledFor(logging.DEBUG): - logger.debug(f"Details: {expensive_serialization(large_object)}") -``` - ### Include Semantic Context in Messages **Constraint:** Log messages MUST include business identifiers using key=value format (e.g., `order_id={order_id}`). diff --git a/rules/index.json b/rules/index.json index f5b2807..52816f0 100644 --- a/rules/index.json +++ b/rules/index.json @@ -613,9 +613,9 @@ }, { "anchor": "python-architecture/constructor-injection-only", - "applies_when": "a Python service class receives a dependency (logger, repository, API client, validator, etc.) through a method parameter, a global module variable, or `self.foo = bar.foo` mutation after construction — instead of through `__init__`.", + "applies_when": "a Python service class receives a dependency (logger, repository, API client, validator, etc.) through a *method parameter* instead of through `__init__`. Related but distinct anti-patterns covered by other rules: dependency from a global module variable (see `python-architecture/main-py-composition-root`); post-construction `self.foo = bar.foo` mutation (see `python-ioc/dependencies-as-private-fields`, which mandates private-field storage that makes mutation socially difficult).", "doc_path": "docs/python-architecture-patterns.md", - "enforcement": "judgment (semantic — requires distinguishing dependency injection from runtime parameter passing)", + "enforcement": "judgment (semantic — requires distinguishing dependency injection from runtime parameter passing. The other two related anti-patterns have their own rules and enforcement paths.)", "id": "python-architecture/constructor-injection-only", "level": "MUST", "owner": "python-architecture-assistant" @@ -633,7 +633,7 @@ "anchor": "python-ioc/dependencies-as-private-fields", "applies_when": "a Python service class stores an injected dependency on `self.` (public attribute) instead of `self._` (single-underscore private convention).", "doc_path": "docs/python-ioc-guide.md", - "enforcement": "judgment (ast-grep follow-up: `assignment` inside `__init__` with left-hand side `self.X` where `X` does not start with `_` and right-hand side is a parameter typed as a `Protocol` / `ABC` interface)", + "enforcement": "judgment (ast-grep first-pass filter: any `assignment` inside `__init__` with left-hand side `self.X` where `X` does not start with `_`. Type-annotation resolution to distinguish service deps from public data fields cannot be done in ast-grep — the agent makes the final call. Overinclusive first-pass is acceptable: most public `self.X` assignments in service classes ARE the smell.)", "id": "python-ioc/dependencies-as-private-fields", "level": "MUST", "owner": "python-architecture-assistant" @@ -649,9 +649,9 @@ }, { "anchor": "python-logging/configure-once-in-main", - "applies_when": "a Python file outside the application entry point (i.e. NOT `main.py` / `__main__.py` / `cli.py` invoked as the entry) calls `logging.basicConfig()` or adds handlers to the root logger.", + "applies_when": "a Python file in *library code* (modules imported by application code, intended for reuse across applications) calls `logging.basicConfig()` or adds handlers to the root logger. **Exempt**: the application entry point (`main.py` / `__main__.py` / `cli.py`) AND application-private helper modules wired from the entry point exclusively for configuration (e.g. `logging_setup.py` — see the next section), since those are part of the entry-point configuration surface, not library code.", "doc_path": "docs/python-logging-guide.md", - "enforcement": "judgment (ast-grep follow-up: `call_expression` matching `logging.basicConfig` outside files identified as entry points by `if __name__ == \"__main__\":` presence; also flag `root_logger.addHandler` in library code)", + "enforcement": "judgment (semantic — distinguishing library code from application-private configuration helpers requires reading the import graph; ast-grep can flag every non-entry-point `logging.basicConfig` call as a first-pass filter, but the agent must rule out the helper-module exemption case)", "id": "python-logging/configure-once-in-main", "level": "MUST", "owner": "python-quality-assistant" @@ -660,7 +660,7 @@ "anchor": "python-logging/lazy-evaluation-for-debug", "applies_when": "a Python `logger.debug(...)` / `logger.log(logging.DEBUG, ...)` call passes an f-string whose interpolation calls an expensive function (serializer, JSON dump, network fetch, large-collection traversal) instead of using `%s` placeholders with deferred arguments.", "doc_path": "docs/python-logging-guide.md", - "enforcement": "judgment (ast-grep partial: `call_expression` named `logger.debug` with an `interpreted_string_literal` first argument containing `f\"...\"` patterns — the \"expensive operation\" judgment is semantic)", + "enforcement": "judgment (ast-grep is a first-pass filter only: `call_expression` named `logger.debug` with an `interpreted_string_literal` first argument matching `f\"...\"`. Trivial f-string interpolations like `logger.debug(f\"count={count}\")` are NOT violations — the rule targets *expensive* interpolations (serializers, JSON dumps, network fetches, large-collection traversals). The agent makes the expensive-vs-trivial judgment after the mechanical filter.)", "id": "python-logging/lazy-evaluation-for-debug", "level": "MUST", "owner": "python-quality-assistant"