diff --git a/README.md b/README.md index d67941d..0b8d312 100644 --- a/README.md +++ b/README.md @@ -58,13 +58,15 @@ Design the interface for this service layer ## Skill Structure Each skill contains: -- `SKILL.md` - Entrypoint loaded into agent context -- `README.md` - Human-facing overview -- `metadata.json` - Version, abstract, references -- `AGENTS.md` - Compiled document with all rules expanded -- `rules/` - Individual rule files (one per rule) - -See `AGENTS.md` at the repo root for guidance on authoring new rules or sections. +- `SKILL.md` - Entrypoint loaded into agent context (quick reference) +- `README.md` - Human-facing overview and authoring workflow +- `metadata.json` - Version, abstract, references, Python version floor +- `AGENTS.md` - (generated) Compiled document with all rules expanded +- `test-cases.json` - (generated) LLM evaluation data extracted from rule examples +- `rules/` - Individual rule files (one per rule), plus `_sections.md` and `_template.md` +- `src/` - Build, validate, and extract-tests scripts (`build.py`, `validate.py`, `extract_tests.py`) + +`AGENTS.md` and `test-cases.json` are generated outputs — do not edit them by hand. See the per-skill README and `AGENTS.md` at the repo root for guidance on authoring new rules or sections. ## License diff --git a/skills/python-best-practices/AGENTS.md b/skills/python-best-practices/AGENTS.md index b2da98f..6e45b6a 100644 --- a/skills/python-best-practices/AGENTS.md +++ b/skills/python-best-practices/AGENTS.md @@ -1,6 +1,6 @@ # Python Best Practices -**Version 1.0.0** +**Version 1.1.0** Python Best Practices April 2026 @@ -14,7 +14,7 @@ April 2026 ## Abstract -Comprehensive Python software engineering guidelines designed for AI agents. Contains 50+ rules across 8 categories, prioritized by impact from critical (data modeling, type safety) to low (import hygiene). Each rule names the failure mode agents tend toward, shows incorrect and correct code, and explains the payoff. Rules are derived from real PR review patterns and production experience. +Comprehensive Python software engineering guidelines designed for AI agents. 60+ rules across 8 categories, prioritized by impact from critical (data modeling, type safety) to low (import hygiene). Each rule names the failure mode agents tend toward, shows incorrect and correct code, cites primary-source references where the rule depends on language or library behavior, and explains the payoff. Rules assume Python 3.11+ as a baseline; rules that depend on a higher version (e.g., 3.13 for warnings.deprecated) are tagged accordingly. --- @@ -25,47 +25,54 @@ Comprehensive Python software engineering guidelines designed for AI agents. Con - 1.2 [Create Explicit Variants Instead of Mode Flags](#12-create-explicit-variants-instead-of-mode-flags) - 1.3 [Delete Dead Variants](#13-delete-dead-variants) - 1.4 [Derive, Don't Store](#14-derive-dont-store) - - 1.5 [Encapsulate Mutable State in the Smallest Possible Scope](#15-encapsulate-mutable-state-in-the-smallest-possible-scope) - - 1.6 [Phase Related Optional Fields Into Nested Structs](#16-phase-related-optional-fields-into-nested-structs) - - 1.7 [Pick a Mutation Contract](#17-pick-a-mutation-contract) - - 1.8 [Use Discriminated Unions Over Optional Bags](#18-use-discriminated-unions-over-optional-bags) + - 1.5 [Encapsulate Mutable State in the Narrowest Clear Scope](#15-encapsulate-mutable-state-in-the-narrowest-clear-scope) + - 1.6 [Never Use Mutable Default Arguments](#16-never-use-mutable-default-arguments) + - 1.7 [Phase Related Optional Fields Into Nested Structs](#17-phase-related-optional-fields-into-nested-structs) + - 1.8 [Pick a Mutation Contract](#18-pick-a-mutation-contract) + - 1.9 [Use Discriminated Unions Over Optional Bags](#19-use-discriminated-unions-over-optional-bags) + - 1.10 [Use Timezone-Aware Datetimes at Boundaries](#110-use-timezone-aware-datetimes-at-boundaries) + - 1.11 [Use a Sentinel Object When None Is a Real Domain Value](#111-use-a-sentinel-object-when-none-is-a-real-domain-value) 2. [Type Safety](#2-type-safety) — **CRITICAL** - 2.1 [Avoid Any Annotations](#21-avoid-any-annotations) - 2.2 [Fix Type Definitions Instead of cast()](#22-fix-type-definitions-instead-of-cast) - 2.3 [Fix Type Errors, Don't Ignore Them](#23-fix-type-errors-dont-ignore-them) - 2.4 [Narrow Type Signatures to Runtime Reality](#24-narrow-type-signatures-to-runtime-reality) - - 2.5 [Remove Redundant Optional Annotations](#25-remove-redundant-optional-annotations) + - 2.5 [Remove Redundant `| None` When Values Are Guaranteed](#25-remove-redundant-none-when-values-are-guaranteed) - 2.6 [Trust the Type Checker — Remove Redundant Runtime Checks](#26-trust-the-type-checker-remove-redundant-runtime-checks) - 2.7 [Use Literal Types for Fixed String Sets](#27-use-literal-types-for-fixed-string-sets) - 2.8 [Use TYPE_CHECKING for Optional Dependencies](#28-use-typechecking-for-optional-dependencies) - 2.9 [Use TypedDict or Dataclass Instead of dict[str, Any]](#29-use-typeddict-or-dataclass-instead-of-dictstr-any) - 2.10 [Use isinstance() for Type Checking, Not hasattr/getattr](#210-use-isinstance-for-type-checking-not-hasattrgetattr) 3. [API Design](#3-api-design) — **HIGH** - - 3.1 [Don't Access Private Attributes](#31-dont-access-private-attributes) - - 3.2 [Instance Methods for State, Module Functions for Pure Logic](#32-instance-methods-for-state-module-functions-for-pure-logic) - - 3.3 [Keep Data Models Flat and Non-Redundant](#33-keep-data-models-flat-and-non-redundant) - - 3.4 [Keep Old Names as Deprecated Aliases](#34-keep-old-names-as-deprecated-aliases) - - 3.5 [Order Required Fields Before Optional Fields](#35-order-required-fields-before-optional-fields) - - 3.6 [Return New Collections from Transforms](#36-return-new-collections-from-transforms) - - 3.7 [Underscore Prefix for Private Names](#37-underscore-prefix-for-private-names) - - 3.8 [Use Keyword-Only Parameters for Optional Config](#38-use-keyword-only-parameters-for-optional-config) + - 3.1 [Avoid Boolean Flag Parameters in Public APIs](#31-avoid-boolean-flag-parameters-in-public-apis) + - 3.2 [Choose the Simplest Namespace That Matches Ownership and Polymorphism](#32-choose-the-simplest-namespace-that-matches-ownership-and-polymorphism) + - 3.3 [Don't Access Private Attributes](#33-dont-access-private-attributes) + - 3.4 [Keep Data Models Flat and Non-Redundant](#34-keep-data-models-flat-and-non-redundant) + - 3.5 [Keep Old Names as Deprecated Aliases](#35-keep-old-names-as-deprecated-aliases) + - 3.6 [Order Required Fields Before Optional Fields](#36-order-required-fields-before-optional-fields) + - 3.7 [Return New Collections from Transforms](#37-return-new-collections-from-transforms) + - 3.8 [Underscore Prefix for Private Names](#38-underscore-prefix-for-private-names) + - 3.9 [Use Keyword-Only Parameters for Optional Config](#39-use-keyword-only-parameters-for-optional-config) 4. [Error Handling](#4-error-handling) — **HIGH** - 4.1 [Catch Specific Exception Types](#41-catch-specific-exception-types) - 4.2 [Consolidate try/except Blocks with the Same Handler](#42-consolidate-tryexcept-blocks-with-the-same-handler) - 4.3 [Inherit New Exceptions from Existing Base Exceptions](#43-inherit-new-exceptions-from-existing-base-exceptions) - - 4.4 [Preserve Asyncio Cancellation Semantics](#44-preserve-asyncio-cancellation-semantics) - - 4.5 [Trust Validated State — Skip Redundant Defensive Checks](#45-trust-validated-state-skip-redundant-defensive-checks) - - 4.6 [Use !r Format for Identifiers in Error Messages](#46-use-r-format-for-identifiers-in-error-messages) - - 4.7 [Use assert for Invariants, Not RuntimeError](#47-use-assert-for-invariants-not-runtimeerror) - - 4.8 [Use raise ... from to Preserve Exception Causality](#48-use-raise-from-to-preserve-exception-causality) - - 4.9 [Validate Input at System Boundaries](#49-validate-input-at-system-boundaries) + - 4.4 [Never Use Bare `except:`](#44-never-use-bare-except) + - 4.5 [Preserve Asyncio Cancellation Semantics](#45-preserve-asyncio-cancellation-semantics) + - 4.6 [Trust Validated State Within the Same Trust Domain](#46-trust-validated-state-within-the-same-trust-domain) + - 4.7 [Use !r Format for Identifiers in Error Messages](#47-use-r-format-for-identifiers-in-error-messages) + - 4.8 [Use assert Only for Debug-Only Internal Invariants](#48-use-assert-only-for-debug-only-internal-invariants) + - 4.9 [Use assert_never for Exhaustiveness Checks](#49-use-assertnever-for-exhaustiveness-checks) + - 4.10 [Use raise ... from to Preserve Exception Causality](#410-use-raise-from-to-preserve-exception-causality) + - 4.11 [Use with / async with for Resource Lifetimes](#411-use-with-async-with-for-resource-lifetimes) + - 4.12 [Validate Input at System Boundaries](#412-validate-input-at-system-boundaries) 5. [Code Simplification](#5-code-simplification) — **MEDIUM-HIGH** - 5.1 [Extract Helpers After 2+ Occurrences](#51-extract-helpers-after-2-occurrences) - 5.2 [Flatten Nested if Statements Into and Conditions](#52-flatten-nested-if-statements-into-and-conditions) - 5.3 [Inline Single-Use Intermediate Variables](#53-inline-single-use-intermediate-variables) - 5.4 [Remove Commented-Out and Dead Code](#54-remove-commented-out-and-dead-code) - 5.5 [Return Early to Flatten Control Flow](#55-return-early-to-flatten-control-flow) - - 5.6 [Use @cached_property for Expensive Derived Attributes](#56-use-cachedproperty-for-expensive-derived-attributes) + - 5.6 [Use @cached_property Only When the Instance Supports It](#56-use-cachedproperty-only-when-the-instance-supports-it) - 5.7 [Use Comprehensions Over for+append Loops](#57-use-comprehensions-over-forappend-loops) - 5.8 [Use any() / all() Over Boolean-Flag Loops](#58-use-any-all-over-boolean-flag-loops) - 5.9 [Use x or default for Fallback Values](#59-use-x-or-default-for-fallback-values) @@ -74,8 +81,8 @@ Comprehensive Python software engineering guidelines designed for AI agents. Con - 6.2 [Combine Filter and Map Into One Pass](#62-combine-filter-and-map-into-one-pass) - 6.3 [Compile Static Regex Patterns at Module Level](#63-compile-static-regex-patterns-at-module-level) - 6.4 [Define TypeAdapter Instances at Module Level](#64-define-typeadapter-instances-at-module-level) - - 6.5 [Use Generators for Streaming Iteration](#65-use-generators-for-streaming-iteration) - - 6.6 [Use Tuple Syntax in isinstance() Checks](#66-use-tuple-syntax-in-isinstance-checks) + - 6.5 [Prefer Tuple Syntax in isinstance() Only on Profiled Hot Paths](#65-prefer-tuple-syntax-in-isinstance-only-on-profiled-hot-paths) + - 6.6 [Stream with Generators When Memory or First-Result Latency Matters](#66-stream-with-generators-when-memory-or-first-result-latency-matters) - 6.7 [Use functools.lru_cache for Pure Functions](#67-use-functoolslrucache-for-pure-functions) - 6.8 [Use set for Repeated Membership Checks](#68-use-set-for-repeated-membership-checks) 7. [Naming](#7-naming) — **MEDIUM** @@ -87,10 +94,11 @@ Comprehensive Python software engineering guidelines designed for AI agents. Con - 7.6 [Use UPPER_CASE for Module Constants](#76-use-uppercase-for-module-constants) 8. [Imports & Structure](#8-imports-structure) — **LOW-MEDIUM** - 8.1 [Handle Optional Dependencies Explicitly](#81-handle-optional-dependencies-explicitly) - - 8.2 [No Duplicate Imports](#82-no-duplicate-imports) - - 8.3 [Place Imports at the Top of the File](#83-place-imports-at-the-top-of-the-file) - - 8.4 [Remove Unused Imports](#84-remove-unused-imports) - - 8.5 [Scope Helpers and Constants to Their Usage Site](#85-scope-helpers-and-constants-to-their-usage-site) + - 8.2 [Keep Modules Cheap to Import](#82-keep-modules-cheap-to-import) + - 8.3 [No Duplicate Imports](#83-no-duplicate-imports) + - 8.4 [Place Imports at the Top of the File](#84-place-imports-at-the-top-of-the-file) + - 8.5 [Remove Unused Imports](#85-remove-unused-imports) + - 8.6 [Scope Helpers and Constants to Their Usage Site](#86-scope-helpers-and-constants-to-their-usage-site) --- @@ -309,13 +317,24 @@ The answer is now computed from evidence that already exists. No sync required **The debugging payoff:** pure derivation means tests become data-in, answer-out. Load fixtures, call the function, assert the result. No mocks, no timing reproduction. -### 1.5 Encapsulate Mutable State in the Smallest Possible Scope +### 1.5 Encapsulate Mutable State in the Narrowest Clear Scope **Impact: HIGH (limits the blast radius of state mutations)** -If mutable state must exist, trap it where only the code that needs it can see it. A closure is better than an instance attribute; an instance attribute is better than a module-level global. Agents default to the loosest scope — push back. +If mutable state must exist, give it the narrowest scope where the code that needs it is still **clear**. The principle is "narrowest *clear* scope," not "always closures over instance attributes." A closure can be the right answer when the state is small, the interface is one or two callables, and there's nothing else to inspect or test. An instance attribute is the right answer when the state belongs to a domain object with identity, when multiple methods need to share it, or when you want it to be easy to inspect, type, serialize, or mock in tests. -**Incorrect (state visible to every method on the class):** +**Pick the smallest scope where the surrounding code still reads naturally:** + +| Scope | Use when | +|-------|----------| +| Local variable | State lives entirely inside one function call | +| Closure | A small handle of 1–2 callables; state must outlive a single call but doesn't need identity | +| Private instance attribute (`_name`) | State belongs to a domain object; multiple methods read/write it; you want introspection, typing, and serialization | +| Module-level global | Genuinely process-wide state — caches, registries (rare; prefer dependency injection) | + +Module-level globals deserve the most pushback. Closures and instance attributes are both legitimate; the choice depends on whether the state has identity worth naming. + +**Incorrect (state visible to every method on the class — too wide):** ```python from typing import Callable @@ -324,30 +343,22 @@ class DebouncedWriter: def __init__(self, callback: Callable[[], None], delay_ms: int = 300): self._callback = callback self._delay_ms = delay_ms - self._timeout_handle: TimerHandle | None = None # visible to all methods - - def queue_send(self, text: str) -> None: - # can touch _timeout_handle - ... - - def flush_now(self) -> None: - # can touch _timeout_handle - ... + self._timeout_handle: TimerHandle | None = None # touched by every method - def something_else(self) -> None: - # can also touch _timeout_handle — and nothing prevents a bug here - ... + def queue_send(self, text: str) -> None: ... + def flush_now(self) -> None: ... + def something_else(self) -> None: ... # nothing prevents a bug here ``` -Any method — including new ones added later — can read or mutate `_timeout_handle`. That's how invariants rot. +If only `queue_send` and `flush_now` need `_timeout_handle`, every other method is a potential source of a state bug. -**Correct (state trapped in a closure):** +**Correct option A (closure — state trapped behind a small handle):** ```python from dataclasses import dataclass from typing import Callable -@dataclass +@dataclass(frozen=True) class DebouncedAction: trigger: Callable[[], None] clear: Callable[[], None] @@ -359,12 +370,12 @@ def create_debounced_action(callback: Callable[[], None], delay_ms: int = 300) - nonlocal timeout if timeout is not None: timeout.cancel() - timeout = schedule_after(delay_ms, lambda: _fire(callback)) + timeout = schedule_after(delay_ms, _fire) - def _fire(cb: Callable[[], None]) -> None: + def _fire() -> None: nonlocal timeout timeout = None - cb() + callback() def clear() -> None: nonlocal timeout @@ -375,11 +386,138 @@ def create_debounced_action(callback: Callable[[], None], delay_ms: int = 300) - return DebouncedAction(trigger=trigger, clear=clear) ``` -Nothing outside the closure can reach `timeout`. The interface is two functions; the state is invisible. +Good fit when the only surface is `trigger` and `clear`, and nothing else needs to inspect `timeout`. + +**Correct option B (small focused class — when identity, inspection, or tests matter):** + +```python +from dataclasses import dataclass, field +from typing import Callable + +@dataclass +class DebouncedAction: + callback: Callable[[], None] + delay_ms: int = 300 + _timeout: TimerHandle | None = field(default=None, init=False, repr=False) + + def trigger(self) -> None: + if self._timeout is not None: + self._timeout.cancel() + self._timeout = schedule_after(self.delay_ms, self._fire) + + def _fire(self) -> None: + self._timeout = None + self.callback() + + def clear(self) -> None: + if self._timeout is not None: + self._timeout.cancel() + self._timeout = None +``` + +Good fit when: + +- Tests want to assert on `_timeout` being `None` +- A debugger should be able to print the object meaningfully +- Subclassing or replacing `_fire` matters +- The object will be serialized, logged, or compared + +Both versions are *narrower* than the original — neither lets unrelated methods touch the timer. The closure isn't categorically better; it's the right call when the surface is tiny and identity is irrelevant. + +**Heuristics for picking:** + +- One or two callables in the public interface, no introspection needed → closure +- Several methods sharing state, identity matters, tests want to peek → focused class with `_private` attributes +- State spans modules → reconsider the design before reaching for a module global + +**The wrong answer is a wide-open class.** Mutable state on a class that lets every method touch it is how invariants rot — regardless of whether the alternative is a closure or a smaller class. + +### 1.6 Never Use Mutable Default Arguments + +**Impact: CRITICAL (prevents shared-state bugs across calls and instances)** + +A default argument is evaluated **once**, when the `def`/class statement runs — not each call. A mutable default (`[]`, `{}`, `set()`, a dataclass instance) is therefore **shared across every call** that doesn't override it. The result is a footgun where appending to the "default" list on one call mutates the default for every subsequent call. The same trap exists for dataclass and Pydantic field defaults. + +Always use `None` (or a sentinel) and construct the mutable inside the body, or use `default_factory` for dataclasses / Pydantic fields. + +**Incorrect (function default — list shared across calls):** + +```python +def append_item(item: int, items: list[int] = []) -> list[int]: + items.append(item) + return items + +append_item(1) # [1] +append_item(2) # [1, 2] ← surprise: same list as before +append_item(3) # [1, 2, 3] +``` + +The `[]` was evaluated once at function-definition time. Every call without an explicit `items=` mutates the same object. + +**Correct (sentinel + per-call construction):** -**When a class is the right tool:** when state belongs to a domain object with identity (a `User`, a `Session`), or when you need multiple methods to share state as a coherent unit. Then the state belongs on the instance — but still as `_private` attributes, not public ones. +```python +def append_item(item: int, items: list[int] | None = None) -> list[int]: + if items is None: + items = [] + items.append(item) + return items -### 1.6 Phase Related Optional Fields Into Nested Structs +append_item(1) # [1] +append_item(2) # [2] ← fresh list per call +``` + +**Incorrect (dataclass — bare mutable default raises `ValueError`, but tempting alternatives are bugs):** + +```python +from dataclasses import dataclass + +@dataclass +class User: + tags: list[str] = [] # ValueError: mutable default ... is not allowed: use default_factory +``` + +The dataclass decorator catches the obvious case. The dangerous variant is sneaking the same list past the check via a class attribute or a shared object — both of which produce the same shared-state bug at runtime. + +**Correct (dataclass — `default_factory`):** + +```python +from dataclasses import dataclass, field + +@dataclass +class User: + tags: list[str] = field(default_factory=list) + metadata: dict[str, str] = field(default_factory=dict) +``` + +`field(default_factory=list)` calls `list()` once per instance, giving each `User` its own list. + +**Incorrect (Pydantic — sharing a list across instances):** + +```python +from pydantic import BaseModel + +class Config(BaseModel): + tags: list[str] = [] # Pydantic deep-copies, but rely on intent, not accident +``` + +Pydantic v2 actually deep-copies the default for each instance, so this happens to work — but the intent is unclear, and the behavior depends on the Pydantic version. Make the factory explicit so future readers (and the type checker) see what you meant. + +**Correct (Pydantic — `Field(default_factory=...)`):** + +```python +from pydantic import BaseModel, Field + +class Config(BaseModel): + tags: list[str] = Field(default_factory=list) + settings: dict[str, str] = Field(default_factory=dict) +``` + +**Heuristic:** if the default value would compare `==` to itself across calls only because it's the *same object*, it's mutable — use `None` + body construction (functions) or `default_factory` (dataclasses, Pydantic). Tuples, frozensets, strings, ints, `None`, and `frozen=True` dataclasses are safe to use directly because they can't be mutated. + +**`from __future__ import annotations` does not help here.** The default-value evaluation rule is unrelated to annotation evaluation; the trap fires either way. + +### 1.7 Phase Related Optional Fields Into Nested Structs **Impact: HIGH (one optional check instead of eight)** @@ -445,7 +583,7 @@ Now consumers check one optional: `if profile.billing is not None: use profile.b **Heuristic:** if three or more optional fields are always set or always unset together, they belong in a nested struct. -### 1.7 Pick a Mutation Contract +### 1.8 Pick a Mutation Contract **Impact: HIGH (prevents ambiguous caller expectations)** @@ -487,7 +625,7 @@ def with_pending_action(state: AppState, action: str) -> AppState: The contract should be obvious from the name and signature without reading the body. -### 1.8 Use Discriminated Unions Over Optional Bags +### 1.9 Use Discriminated Unions Over Optional Bags **Impact: CRITICAL (makes impossible states unrepresentable)** @@ -539,10 +677,213 @@ PaymentState = PaymentIdle | PaymentProcessing | PaymentSettled Now `match payment.status:` narrows exactly, `transaction_id` is non-optional on the variants that have it, and impossible combinations (idle with a transaction ID, settled without a timestamp) are unrepresentable. -**With Pydantic:** use `Field(discriminator="status")` and a `status: Literal[...]` tag on each variant — Pydantic will validate and narrow automatically. +**With Pydantic** *(applicability: pydantic)*: use `Field(discriminator="status")` and a `status: Literal[...]` tag on each variant — Pydantic will validate and narrow automatically. **Null over sentinels:** don't invent `"none"` action values. `pending_action: PendingAction | None` beats `pending_action: Literal["none", "confirm-address", "select-shipping"]`. Absence is not an action. +### 1.10 Use Timezone-Aware Datetimes at Boundaries + +**Impact: HIGH (prevents off-by-hours bugs across timezones, daylight saving, and storage)** + +A `datetime` with no `tzinfo` is **naive**: it has no opinion about which timezone it represents. Two naive datetimes that look identical may refer to different absolute moments. Naive datetimes leak into databases, JSON payloads, log lines, and inter-service messages and cause off-by-hours bugs that surface during DST transitions, on a different host, or when a user travels. + +The rule: at any boundary the value crosses (HTTP, DB, queue, file format, log line, comparison with another datetime), the datetime must be **timezone-aware**. Inside a tight piece of business logic, naive is acceptable only if every value in scope shares the same explicit assumption — and even then, attaching the timezone is usually clearer. + +**Default to UTC for storage and transport. Convert to local timezones only at display.** + +**Incorrect (`datetime.utcnow()` returns a naive datetime — silently loses the "UTC" claim):** + +```python +from datetime import datetime + +def stamp() -> datetime: + return datetime.utcnow() # naive! DeprecationWarning in 3.12+ +``` + +`datetime.utcnow()` is deprecated in Python 3.12 precisely because it returns a *naive* datetime that callers misuse as if it were UTC-aware. A serializer that interprets naive as local time will write the wrong value to the database. + +**Incorrect (`datetime.now()` is naive and host-local):** + +```python +from datetime import datetime + +start = datetime.now() # naive, in the host's local timezone +log.info("started", start=start) # serializes ambiguously +``` + +The same code on two hosts in different timezones records different timestamps for the same event. + +**Incorrect (mixing naive and aware in comparisons — `TypeError`):** + +```python +from datetime import datetime, timezone + +stored = datetime(2026, 4, 17, 12, 0) # naive +now = datetime.now(timezone.utc) # aware +if stored < now: # TypeError! + ... +``` + +The interpreter refuses to compare naive and aware datetimes — a guard against a class of bugs that would otherwise be silent. + +**Correct (UTC at every boundary):** + +```python +from datetime import datetime, timezone + +def stamp() -> datetime: + return datetime.now(timezone.utc) # aware, unambiguous + +start = datetime.now(timezone.utc) +log.info("started", start=start.isoformat()) # "2026-04-17T12:00:00+00:00" +``` + +`datetime.now(timezone.utc)` is the modern replacement for `datetime.utcnow()`. The result is aware and round-trips through `isoformat()` / `fromisoformat()` cleanly. + +**Correct (named local timezone via `zoneinfo` for display):** + +```python +from datetime import datetime, timezone +from zoneinfo import ZoneInfo + +stored = datetime.now(timezone.utc) # store in UTC +display = stored.astimezone(ZoneInfo("America/Los_Angeles")) # convert at display +print(display.strftime("%Y-%m-%d %H:%M %Z")) +``` + +`zoneinfo` (Python 3.9+, PEP 615) reads from the system tzdata; it handles DST and historical offsets correctly. Use named zones (`"America/Los_Angeles"`), not raw offsets (`-08:00`), so DST transitions resolve. + +**Correct (parsing user/API input — fail loudly on missing timezone):** + +```python +from datetime import datetime, timezone + +def parse_iso(s: str) -> datetime: + dt = datetime.fromisoformat(s) + if dt.tzinfo is None: + raise ValueError(f"datetime {s!r} is missing a timezone offset") + return dt.astimezone(timezone.utc) +``` + +If your callers can send naive datetimes, decide once whether to reject them or to assume a fixed zone — but never *silently* treat naive as UTC. + +**Pydantic / dataclasses:** + +```python +from datetime import datetime, timezone +from pydantic import BaseModel, AwareDatetime + +class Event(BaseModel): + occurred_at: AwareDatetime # Pydantic v2: rejects naive datetimes at validation +``` + +`pydantic.AwareDatetime` enforces the rule at the model boundary. The standard library doesn't ship a "must be aware" annotation; encode the constraint with a validator or rely on Pydantic. + +**Database guidance:** + +- PostgreSQL: use `TIMESTAMPTZ` (stores UTC). Driver returns aware datetimes. +- SQLite / MySQL: store ISO-8601 strings with `+00:00`, or store epoch milliseconds. +- ORMs: configure timezone-aware columns explicitly; defaults vary. + +**When naive is acceptable:** + +- Pure date arithmetic where time-of-day doesn't matter (`date`, not `datetime`) +- A small block of business logic where every value is naive and the timezone is documented in scope +- Integrating with a legacy system whose contract is naive — but convert at the boundary on the way out + +**Heuristic:** if the datetime is going to live longer than the function it's created in, it should be aware. Naive datetimes are a sharp local tool, never a transport format. + +### 1.11 Use a Sentinel Object When None Is a Real Domain Value + +**Impact: MEDIUM-HIGH (distinguishes "no value passed" from "None passed deliberately")** + +When `None` carries semantic meaning in your domain — "the user explicitly cleared this field," "no parent," "no assignee" — you can no longer use `None` as a "not provided" default. Reach for a private sentinel object instead. This complements `types-remove-redundant-optional`: that rule says drop `| None` when `None` is impossible; this rule says use a sentinel when `None` is meaningfully different from "not passed." + +PEP 661 documents the pattern (it didn't standardize a syntax, but the idiom is universal). The sentinel is a unique object you compare with `is`, never with `==`. + +**Incorrect (using `None` as both "absent" and "explicitly cleared"):** + +```python +def update_user(user_id: str, nickname: str | None = None) -> User: + user = db.get(user_id) + user.nickname = nickname # was the caller clearing the nickname, + db.save(user) # or did they just not pass it? + return user + +update_user("u1") # didn't touch nickname? cleared it? +update_user("u1", nickname=None) # same call — same ambiguity +update_user("u1", nickname="bob") # this one is clear +``` + +There is no way for the function to tell "the caller didn't mention nickname" from "the caller wants to clear it." That ambiguity has bitten every PATCH-style API ever written. + +**Correct (sentinel default + `None` meaning "clear"):** + +```python +from typing import Final + +class _Unset: + def __repr__(self) -> str: + return "" + +UNSET: Final = _Unset() + +def update_user( + user_id: str, + nickname: str | None | _Unset = UNSET, +) -> User: + user = db.get(user_id) + if nickname is not UNSET: + user.nickname = nickname # may be None (cleared) or a real string + db.save(user) + return user + +update_user("u1") # nickname untouched +update_user("u1", nickname=None) # nickname cleared +update_user("u1", nickname="bob") # nickname set to "bob" +``` + +Compare with `is`, not `==`, so callers can't accidentally pass an object that compares equal. + +**For Pydantic models — same pattern, same payoff.** Distinguishing "field omitted from PATCH payload" vs. "field set to null" is the canonical use case: + +```python +from typing import Any +from pydantic import BaseModel, Field + +class _Unset: + def __repr__(self) -> str: + return "" + +UNSET: Any = _Unset() # Any so it satisfies any field annotation + +class UserPatch(BaseModel): + nickname: str | None = Field(default=UNSET) + email: str = Field(default=UNSET) + + def changes(self) -> dict[str, object]: + return {k: v for k, v in self.model_dump().items() if v is not UNSET} +``` + +Now `UserPatch(nickname=None).changes() == {"nickname": None}` (clear) and `UserPatch().changes() == {}` (untouched). + +**`typing` exposes `Sentinel` (3.13+, PEP 661 follow-up).** When available, you can shorten the boilerplate: + +```python +# Python 3.13+ (proposed; check your interpreter) +from typing import Sentinel + +UNSET = Sentinel("UNSET") +``` + +Until that lands universally, the small `_Unset` class above is the portable form. + +**Don't use generic objects as sentinels.** `_UNSET = object()` works, but it gives no help to readers, type checkers, or debuggers. A small named class with a `__repr__` makes tracebacks readable. + +**Don't reach for sentinels when `None` is fine.** If `None` already means "absent" and there's no separate "explicitly cleared" state to distinguish, plain `nickname: str | None = None` is the right answer. The sentinel earns its complexity only when both meanings need to coexist. + +**Heuristic:** if your function or model needs to distinguish three states — "not provided," "provided as None," "provided as a real value" — you need a sentinel. Two states (`None` vs. value) is just `Optional`. + ## 2. Type Safety **Impact: CRITICAL** @@ -756,7 +1097,7 @@ def render_tool_result(part: ToolPart) -> str: `assert_never` ensures that if `ToolPart` gains a new variant, every `match` on it is re-examined. -### 2.5 Remove Redundant Optional Annotations +### 2.5 Remove Redundant `| None` When Values Are Guaranteed **Impact: MEDIUM (eliminates false uncertainty in the type signature)** @@ -1089,60 +1430,118 @@ When `part.kind` is a `Literal` discriminator on a `Union`, `match` narrows each Interface decisions that compound over years. Keyword-only parameters, private underscores, immutable transforms. The difference between an API that ages well and one that accumulates compatibility shims. -### 3.1 Don't Access Private Attributes +### 3.1 Avoid Boolean Flag Parameters in Public APIs -**Impact: HIGH (prevents breakage when internals change)** +**Impact: HIGH (prevents call sites that read like "do_thing(thing, True, False)")** -`_prefixed` names are the author's contract: "this is internal, it may change." Reaching into another module's or class's private attributes couples your code to implementation details you weren't invited into. Use the public API, or ask the owner to expose what you need. +A boolean parameter is a binary mode switch hiding behind a generic type. The call site `download(url, True, False, True)` is unreadable, the function body branches on the flag with two near-duplicate code paths, and adding a third mode later requires breaking the API. This is the function-level cousin of `data-explicit-variants`: when behavior meaningfully changes on a flag, prefer split functions or a `Literal`/`Enum` parameter. -**Incorrect (poking at private state):** +**Incorrect (boolean flags — call sites lose meaning):** ```python -from some_lib import Client +def export_report(rows: list[Row], to_csv: bool = True, compress: bool = False) -> bytes: + if to_csv: + data = render_csv(rows) + else: + data = render_json(rows) + if compress: + data = gzip.compress(data) + return data -client = Client() -# peeking at a private attribute because there's no public way -retry_count = client._retry_state["count"] -client._pool.clear() # mutating private state +export_report(rows, True, False) # what does True/False mean here? +export_report(rows, False, True) # JSON, compressed? CSV, compressed? Reader can't tell. ``` -Next version of `some_lib` renames `_retry_state` to `_retries` (it's private, they're allowed to) — your code breaks with no warning. Or worse, `_pool.clear()` no longer does what you assumed, and you corrupt state silently. +The function body is two if-branches stacked, the call sites carry no information, and any third format (Parquet, XML) means another bool — `to_csv: bool, to_json: bool, to_parquet: bool` is incoherent. -**Correct (use the public API):** +**Correct option A (split into separate functions when bodies barely overlap):** ```python -from some_lib import Client +def export_csv(rows: list[Row]) -> bytes: ... +def export_json(rows: list[Row]) -> bytes: ... -client = Client() -retry_count = client.stats.retries # public property -client.reset_pool() # public method +def with_compression(data: bytes) -> bytes: + return gzip.compress(data) + +# call site +data = with_compression(export_csv(rows)) ``` -If `some_lib` doesn't expose what you need, open an issue or PR. Using `_private` is a workaround, not a fix. +Each function does one thing. Adding `export_parquet` is additive, not breaking. Compression composes orthogonally. -**Inside your own code:** same rule applies between modules. If `module_a` finds itself reaching into `module_b._helpers`, the helper probably shouldn't be private — or `module_a` shouldn't need it. +**Correct option B (`Literal` parameter when the modes share most of the body):** -**The exception:** testing your own internals. Unit tests for a class may legitimately assert on `_private` state. Even then, prefer testing through the public interface when feasible — tests that poke at internals are brittle to refactoring. +```python +from typing import Literal -**Double underscore (`__name`) is stronger:** Python name-mangles `__name` to `_ClassName__name`, making accidental access even harder. Use it for attributes you're committed to keeping inaccessible. +Format = Literal["csv", "json", "parquet"] -### 3.2 Instance Methods for State, Module Functions for Pure Logic +def export_report(rows: list[Row], format: Format, *, compress: bool = False) -> bytes: + match format: + case "csv": data = render_csv(rows) + case "json": data = render_json(rows) + case "parquet": data = render_parquet(rows) + return gzip.compress(data) if compress else data -**Impact: MEDIUM (avoids unnecessary coupling while enabling polymorphism)** +export_report(rows, format="csv", compress=True) +``` + +Adding a fourth format is a one-line change to the `Literal`; the call sites read meaningfully (`format="parquet"` instead of `True, False, True`). + +**Correct option C (`Enum` when the modes carry behavior or constants):** + +```python +from enum import Enum + +class CompressionLevel(Enum): + NONE = 0 + FAST = 1 + BEST = 9 -Agents tend to put everything on classes because "that's how OOP works" — or conversely, make everything a module-level function because "pure is better." The right call depends on whether the function genuinely needs `self` or enables polymorphism. +def export_report(rows: list[Row], *, level: CompressionLevel = CompressionLevel.NONE) -> bytes: + data = render_csv(rows) + if level is CompressionLevel.NONE: + return data + return gzip.compress(data, compresslevel=level.value) +``` -**Use an instance method when:** -- The function accesses `self` attributes -- It's a natural operation on the object (the method *is* part of the object's interface) -- Subclasses will override it (polymorphism) +The enum gives each variant a name *and* a meaningful value. Type checkers narrow on `is` comparisons. -**Use a module-level function when:** -- Nothing about the logic depends on instance state -- The function is a pure utility that happens to take an object of that class -- Multiple classes could reasonably use the same helper +**`bool` parameters that are genuinely binary toggles are still okay** — but only when: -**Incorrect (module-level function awkwardly threading state):** +- The flag is keyword-only (use `*` per `api-keyword-only-params`) +- The name clearly answers "what does True mean?" (`include_archived=True`, `strict=True`, `dry_run=True`) +- There's no plausible third mode coming +- The body doesn't fork into two near-duplicate paths + +```python +def list_users(*, include_archived: bool = False) -> list[User]: + if include_archived: + return query_all_users() + return query_active_users() +``` + +`include_archived=True` reads at the call site. The body is genuinely a small branch on a single SQL filter. + +**Heuristic:** read your call sites out loud. `export_report(rows, True, False)` fails the test. `export_report(rows, format="csv", compress=True)` passes. If you hear positional booleans, the API needs splitting or a `Literal`. + +### 3.2 Choose the Simplest Namespace That Matches Ownership and Polymorphism + +**Impact: MEDIUM (avoids unnecessary coupling without forcing a binary choice)** + +Python lets the same logic live as a module-level function, an instance method, a `@classmethod`, a `@staticmethod`, a method on a `Protocol`, or a method on a `dataclass`. None of these is universally right. Pick the smallest namespace that captures **ownership** (does this operation belong to one object?) and **polymorphism** (will multiple types provide their own version?). + +A useful decision order, from simplest to most coupled: + +1. **Module-level function** — when the logic is a pure utility that operates on its arguments and doesn't need to be overridden. +2. **Instance method** — when the logic naturally reads as "this object does X" and uses `self`, *or* when subclasses / Protocol implementations need to provide their own version. +3. **`@classmethod`** — alternative constructors, factory methods, things that need the class but not an instance. +4. **`@staticmethod`** — namespace grouping when the helper is conceptually tied to the class but takes no `self`/`cls`. Often a sign a module-level function would do. +5. **Protocol** — when several unrelated types need to provide the same interface and you want structural typing. + +There is no "correct" tier; pick the simplest one that fits. + +**Incorrect (module-level function awkwardly threading state through `user`):** ```python def update_user_preferences(user: User, key: str, value: object) -> None: @@ -1153,9 +1552,9 @@ def get_user_display_name(user: User) -> str: return f"{user.first_name} {user.last_name}" ``` -These both mutate/read `user` state and are core user operations — they belong on `User`. +These mutate or read `user` state, name `user` in their parameter list, and have no second caller type. They belong on `User`. -**Correct (instance methods):** +**Correct (instance methods — ownership matches the object):** ```python class User: @@ -1168,12 +1567,12 @@ class User: return f"{self.first_name} {self.last_name}" ``` -**Incorrect (instance method that doesn't need `self`):** +**Incorrect (instance method that doesn't need `self` and isn't overridden):** ```python class DateFormatter: def format_iso(self, d: date) -> str: - return d.isoformat() # doesn't touch self + return d.isoformat() # `self` is unused ``` **Correct (module-level function):** @@ -1183,11 +1582,80 @@ def format_iso(d: date) -> str: return d.isoformat() ``` -**Extract shared logic to private top-level helpers** when multiple classes need the same computation — don't duplicate it across methods. +If five subclasses of `DateFormatter` are about to override `format_iso` with locale-specific behavior, the method form is correct after all — polymorphism justifies the coupling. -**`@staticmethod` / `@classmethod`:** reach for these sparingly. If a method doesn't need `self` or `cls`, it's usually a module-level function. Reserve them for alternative constructors (`@classmethod`) or namespace-grouped utilities where the class genuinely makes things more discoverable. +**`@classmethod` for alternative constructors:** -### 3.3 Keep Data Models Flat and Non-Redundant +```python +class Event: + def __init__(self, kind: str, payload: dict[str, Any]) -> None: + self.kind = kind + self.payload = payload + + @classmethod + def from_json(cls, raw: str) -> "Event": + data = json.loads(raw) + return cls(kind=data["kind"], payload=data["payload"]) +``` + +`from_json` doesn't need an instance, but it does need the class for subclass-friendly construction. + +**`@staticmethod` is the rarest tier.** If the function takes no `self` and no `cls`, the only reason to attach it to a class is namespacing — and a module-level function is usually cleaner. Reserve `@staticmethod` for cases where the class genuinely makes the helper more discoverable (a small private validator on a model, for example). + +**Protocols when the consumer doesn't need to know the producer:** + +```python +from typing import Protocol + +class JSONSerializable(Protocol): + def to_json(self) -> str: ... + +def write(obj: JSONSerializable, path: Path) -> None: + path.write_text(obj.to_json()) +``` + +Now any type with `to_json` works — no shared base class, no inheritance. + +**Heuristic:** start at module scope. Promote to a method only when ownership or polymorphism *actually* demand it. The cost of starting too coupled (everything on a class) is harder to undo than the cost of starting too loose (a free function you later move). + +### 3.3 Don't Access Private Attributes + +**Impact: HIGH (prevents breakage when internals change)** + +`_prefixed` names are the author's contract: "this is internal, it may change." Reaching into another module's or class's private attributes couples your code to implementation details you weren't invited into. Use the public API, or ask the owner to expose what you need. + +**Incorrect (poking at private state):** + +```python +from some_lib import Client + +client = Client() +# peeking at a private attribute because there's no public way +retry_count = client._retry_state["count"] +client._pool.clear() # mutating private state +``` + +Next version of `some_lib` renames `_retry_state` to `_retries` (it's private, they're allowed to) — your code breaks with no warning. Or worse, `_pool.clear()` no longer does what you assumed, and you corrupt state silently. + +**Correct (use the public API):** + +```python +from some_lib import Client + +client = Client() +retry_count = client.stats.retries # public property +client.reset_pool() # public method +``` + +If `some_lib` doesn't expose what you need, open an issue or PR. Using `_private` is a workaround, not a fix. + +**Inside your own code:** same rule applies between modules. If `module_a` finds itself reaching into `module_b._helpers`, the helper probably shouldn't be private — or `module_a` shouldn't need it. + +**The exception:** testing your own internals. Unit tests for a class may legitimately assert on `_private` state. Even then, prefer testing through the public interface when feasible — tests that poke at internals are brittle to refactoring. + +**Double underscore (`__name`) is stronger:** Python name-mangles `__name` to `_ClassName__name`, making accidental access even harder. Use it for attributes you're committed to keeping inaccessible. + +### 3.4 Keep Data Models Flat and Non-Redundant **Impact: MEDIUM (reduces API surface and prevents field drift)** @@ -1243,7 +1711,7 @@ class ToolCall: **Why it matters:** redundancy means every mutation site has two (or more) places to update. Skipping one creates a drift bug that's only visible when the fields disagree. -### 3.4 Keep Old Names as Deprecated Aliases +### 3.5 Keep Old Names as Deprecated Aliases **Impact: HIGH (enables gradual migration without breakage)** @@ -1259,7 +1727,7 @@ def get_user(user_id: str) -> User: ... def fetch_user(user_id: str) -> User: ... # renamed — v1.0 callers now crash ``` -**Correct (deprecated alias):** +**Correct (deprecated alias with `warnings.warn`):** ```python import warnings @@ -1278,35 +1746,61 @@ def get_user(user_id: str) -> User: Old callers keep working with a warning; new callers use the new name. -**For parameter renames, use `typing.deprecated` (Python 3.13+) or keyword handling:** +**On Python 3.13+, prefer `warnings.deprecated()` for whole functions, classes, and overloads.** PEP 702 added a standard decorator that emits the warning, marks the symbol so static checkers can flag callers, and surfaces the deprecation in IDE tooling. The decorator lives in `warnings`, **not** `typing`: ```python +import warnings # Python 3.13+ + +@warnings.deprecated("get_user is deprecated; use fetch_user instead.") +def get_user(user_id: str) -> User: + return fetch_user(user_id) + + +@warnings.deprecated("LegacyClient is deprecated; use Client instead.") +class LegacyClient(Client): ... +``` + +Type checkers that implement PEP 702 (mypy, pyright) report calls to deprecated names without you having to wire `warnings.warn` by hand. + +**For renamed parameters, `warnings.deprecated()` does not apply** — it decorates whole symbols, not individual parameters. Use a compatibility keyword path plus a runtime warning inside the function: + +```python +import warnings +from typing import Any + +_MISSING: Any = object() + def fetch_user( - user_id: str, + user_id: str = _MISSING, *, timeout: float = 30, - user_id_alt: str | None = None, # old name + user_id_alt: str = _MISSING, # old name; remove in next major ) -> User: - if user_id_alt is not None: + if user_id_alt is not _MISSING: warnings.warn( - "user_id_alt is deprecated; pass user_id instead.", + "the user_id_alt parameter is deprecated; pass user_id instead.", DeprecationWarning, stacklevel=2, ) - user_id = user_id_alt + if user_id is _MISSING: + user_id = user_id_alt + if user_id is _MISSING: + raise TypeError("fetch_user() missing required argument: 'user_id'") ... ``` +The compatibility shim (the old keyword still accepted, then forwarded) is what preserves callers; the `warnings.warn(..., DeprecationWarning, stacklevel=2)` call is what surfaces the migration. + **Deprecation policy:** 1. Add the new name. Old name becomes an alias. -2. Emit a `DeprecationWarning` explaining the migration. +2. Emit a `DeprecationWarning` (via `warnings.warn` or `@warnings.deprecated`) explaining the migration. 3. Document the deprecation in the changelog and docstrings. 4. Remove the alias in a later major version (follow your project's deprecation window — typically one or two releases). **When you can skip the alias:** the function was never part of the documented public API (starts with `_`, not in `__all__`, not in published docs). Internal renames don't need deprecation. -### 3.5 Order Required Fields Before Optional Fields +### 3.6 Order Required Fields Before Optional Fields **Impact: HIGH (Python enforces this at class-definition time)** @@ -1364,7 +1858,7 @@ def connect(port, host="localhost"): ... def connect(*, port, host="localhost", retries): ... ``` -### 3.6 Return New Collections from Transforms +### 3.7 Return New Collections from Transforms **Impact: HIGH (prevents surprising side effects)** @@ -1406,7 +1900,7 @@ Name conventions: **Rule of thumb:** if the function's name is a verb phrase describing a transformation, default to returning new. If it's imperative and clearly a command (`sort`, `apply`, `set`), mutation is expected. -### 3.7 Underscore Prefix for Private Names +### 3.8 Underscore Prefix for Private Names **Impact: HIGH (signals internal API and limits backward-compat obligations)** @@ -1455,7 +1949,7 @@ class Cache: **`__all__` is the contract:** `from mymodule import *` respects `__all__`. Tools like Sphinx and type checkers also use it to determine the public surface. Keep it minimal and accurate. -### 3.8 Use Keyword-Only Parameters for Optional Config +### 3.9 Use Keyword-Only Parameters for Optional Config **Impact: HIGH (prevents breakage when adding or reordering params)** @@ -1514,7 +2008,7 @@ Sloppy exceptions hide bugs; good exceptions localize them. Catch specific types **Impact: HIGH (prevents masking unrelated bugs)** -`except Exception:` catches everything — including the bugs you wanted to see. Agents default to broad handlers because "we should be resilient"; the cost is that `KeyError` from a typo in your own code gets silently swallowed alongside the network timeout you meant to handle. +Catch the specific exception types you actually intend to handle. A broad `except Exception:` catches every regular error in your codebase, including bugs you wanted to see. (For the even worse `except:` with no type at all — which also catches `KeyboardInterrupt` and `SystemExit` — see `error-no-bare-except`.) Agents default to broad handlers because "we should be resilient"; the cost is that `KeyError` from a typo in your own code gets silently swallowed alongside the network timeout you meant to handle. **Incorrect (bare except catches unrelated errors):** @@ -1682,25 +2176,89 @@ except ToolError: # catches ToolRateLimitError too Callers that want to handle rate limits specifically can add `except ToolRateLimitError:` — but existing broad handlers keep working. -**Design the hierarchy deliberately:** +**Design the hierarchy deliberately:** + +```python +class PackageError(Exception): ... # root for everything the package raises +class UserError(PackageError): ... # user-correctable +class ConfigError(UserError): ... +class UsageError(UserError): ... +class SystemError(PackageError): ... # environmental / transient +class NetworkError(SystemError): ... +class TimeoutError(SystemError): ... +``` + +Callers can catch at whichever level of specificity they need. Adding new subtypes is non-breaking. + +**Don't invert the hierarchy:** `class TimeoutError(PackageError)` is fine; `class PackageError(TimeoutError)` is nonsense. The base is the broader category, subclasses are narrower. + +**Use `__init_subclass__` or explicit checks** if you need to prevent direct instantiation of the base — keep the type system as the contract enforcement. + +### 4.4 Never Use Bare `except:` + +**Impact: HIGH (bare except swallows KeyboardInterrupt, SystemExit, and async cancellation)** + +`except:` (with no exception type) catches **`BaseException`** — every exception in the interpreter, including the ones you must not silently swallow: + +- `KeyboardInterrupt` — Ctrl-C +- `SystemExit` — `sys.exit()`, normal interpreter shutdown +- `asyncio.CancelledError` (3.8+) and `BaseExceptionGroup` (3.11+) — async cancellation +- `MemoryError`, `GeneratorExit`, internal interpreter signals + +Bare `except` is broader than `except Exception:`, and the breadth is exactly the problem. PEP 8 calls it out: *"A bare `except:` clause will catch `SystemExit` and `KeyboardInterrupt` exceptions, making it harder to interrupt a program with Control-C."* `flake8` / `ruff` flag it as `E722`. Treat any bare `except:` as a bug. + +**Incorrect (bare except — Ctrl-C cannot interrupt this loop):** + +```python +while True: + try: + process_one() + except: # E722: bare except + log("retrying") + time.sleep(1) +``` + +A user who hits Ctrl-C is ignored. A `sys.exit()` from a child function is ignored. An async `CancelledError` is swallowed and the task hangs. + +**Incorrect (`except BaseException:` — same problem, spelled out):** + +```python +try: + do_work() +except BaseException: # don't catch BaseException directly either + log("done") +``` + +Catching `BaseException` is the explicit form of the same mistake. + +**Correct (catch what you actually intend to handle):** + +```python +while True: + try: + process_one() + except (TimeoutError, ConnectionError) as exc: + log("retrying", exc_info=exc) + time.sleep(1) + # KeyboardInterrupt, SystemExit, CancelledError propagate as they should +``` + +**Correct when you need a true catch-all (last line of defense — log and re-raise):** ```python -class PackageError(Exception): ... # root for everything the package raises -class UserError(PackageError): ... # user-correctable -class ConfigError(UserError): ... -class UsageError(UserError): ... -class SystemError(PackageError): ... # environmental / transient -class NetworkError(SystemError): ... -class TimeoutError(SystemError): ... +def handle_request(req: Request) -> Response: + try: + return process(req) + except Exception: # NOT bare; excludes BaseException-only types + logger.exception("unhandled error in request handler") + raise # never swallow ``` -Callers can catch at whichever level of specificity they need. Adding new subtypes is non-breaking. - -**Don't invert the hierarchy:** `class TimeoutError(PackageError)` is fine; `class PackageError(TimeoutError)` is nonsense. The base is the broader category, subclasses are narrower. +Use `except Exception:` (not bare) at the outermost layer of a request handler, worker loop, or top-level entrypoint where you must log unexpected errors. Always re-raise — see `error-specific-exceptions` for the broader handler discussion, and `error-preserve-cancellation` for why `CancelledError` must reach the event loop. -**Use `__init_subclass__` or explicit checks** if you need to prevent direct instantiation of the base — keep the type system as the contract enforcement. +**The only legitimate use of `except BaseException:`** is in framework-level cleanup code that genuinely must run before the process exits (e.g., flushing logs in a process supervisor) — and even then, the handler must re-raise. If you're not writing that, you don't need it. -### 4.4 Preserve Asyncio Cancellation Semantics +### 4.5 Preserve Asyncio Cancellation Semantics **Impact: HIGH (avoids hung tasks and false-positive review flags)** @@ -1776,30 +2334,57 @@ async def session() -> None: **Review heuristic for `except Exception:` in async code:** flag it for diagnostic precision, client-visible error leaks, or swallowing domain bugs — never for cancellation safety on Python 3.8+. Before claiming otherwise, verify the project's Python version and whether the catch is `Exception` or `BaseException`. -### 4.5 Trust Validated State — Skip Redundant Defensive Checks +### 4.6 Trust Validated State Within the Same Trust Domain + +**Impact: MEDIUM (removes clutter without losing real safety)** + +Once a value has been validated *and the validated object is immutable, locally constructed, and stays inside the same trust domain*, internal helpers can skip re-checking it. Outside that narrow case, defensive checks may still earn their keep — mutable objects can drift, plugin/untyped callers can construct bad instances, and rehydrated objects (from a cache, a queue, the database) cross a trust boundary even if the type name is the same. + +This rule is the cousin of `types-trust-the-checker`. The principle is the same — don't duplicate guarantees the system already provides — but state requires more care than types because state can change after validation. -**Impact: MEDIUM (removes clutter and improves resilience)** +**Trust-domain checklist before deleting a defensive check:** -Once a value has been validated at the boundary, internal code should trust it. Agents tend to add defensive checks "just in case" deep inside the call chain — the cost is noise, false branches, and the impression that validation elsewhere isn't reliable. +1. **Immutability** — the object is frozen, or the field cannot be reassigned after construction. +2. **Locally constructed** — built by code you control, in this process, since the last validation. +3. **No untyped/plugin caller** — no place can produce the type without going through the validator. +4. **No rehydration since validation** — not loaded from cache, queue, RPC, or DB without re-validating. -**Incorrect (re-checking already-validated state):** +Meet all four → trust the invariant. Miss one → keep the check. + +**Incorrect (re-checking validated immutable state inside the same module):** ```python +from pydantic import BaseModel, model_validator + +class ValidatedOrder(BaseModel): + model_config = {"frozen": True} + items: list[Item] + total: int + + @model_validator(mode="after") + def _check(self) -> "ValidatedOrder": + if not self.items: + raise ValueError("order must have items") + if self.total < 0: + raise ValueError("total must be non-negative") + return self + + def fulfill_order(order: ValidatedOrder) -> None: - if order is None: # already enforced by type + if order is None: # type already excludes None raise ValueError("order required") - if not order.items: # already enforced by Pydantic validator + if not order.items: # validator guarantees this raise ValueError("order must have items") - if order.total < 0: # already enforced by validator + if order.total < 0: # validator guarantees this raise ValueError("total must be non-negative") for item in order.items: process(item) ``` -Every one of these checks was already done when `ValidatedOrder` was constructed. Repeating them says "I don't trust the validation." +Frozen + validated + local construction + no rehydration → the checks are noise. -**Correct (trust the invariants):** +**Correct (trust the invariant):** ```python def fulfill_order(order: ValidatedOrder) -> None: @@ -1807,35 +2392,63 @@ def fulfill_order(order: ValidatedOrder) -> None: process(item) ``` -Cleaner, faster, and if the validator changes, this function doesn't need updating. +**Keep defensive checks when any condition fails.** Concrete examples: + +**Mutable object:** + +```python +@dataclass # not frozen +class Cart: + items: list[Item] # callers can mutate after construction + +def checkout(cart: Cart) -> None: + if not cart.items: # KEEP — caller could have cleared the list + raise EmptyCartError() + ... +``` + +**Rehydrated from external storage:** -**When defensive checks are appropriate:** +```python +def replay_from_queue(payload: bytes) -> None: + order = ValidatedOrder.model_validate_json(payload) + # Validator just ran on this newly-constructed object → no extra check needed here. + ... + +def load_from_cache(key: str) -> ValidatedOrder: + raw = cache.get(key) + return ValidatedOrder.model_validate_json(raw) # KEEP validation — cache crossed a boundary +``` + +**Untyped or plugin caller:** + +```python +def run_user_plugin(plugin: Any) -> None: + config = plugin.get_config() + if not isinstance(config, ValidatedConfig): # KEEP — plugin might return anything + raise TypeError("plugin returned non-ValidatedConfig") + ... +``` -- At trust boundaries (first function to touch external data) -- Around code paths that can bypass the validator (direct construction in tests, deserialization shortcuts) -- When the invariant is load-bearing and a bug elsewhere could silently violate it (use `assert` to document) +**Document the invariant once when you do trust it.** A single `assert` (with the caveats from `error-assert-debug-only`) at the entry point can serve as documentation for readers without sprinkling defensive `if` chains through the body: ```python def fulfill_order(order: ValidatedOrder) -> None: - assert order.items, "ValidatedOrder must have items (validator guarantees this)" - # assertion documents the invariant; runs in dev, stripped in production + assert order.items, "ValidatedOrder validator guarantees non-empty items" for item in order.items: process(item) ``` -**Use defaults instead of assertions** when the goal is resilience rather than catching bugs: +**Resilience vs. strictness.** When the goal is "keep running on bad input" rather than "catch a bug," reach for a default rather than a check-and-raise: ```python -# defensive, resilient — use when the system should keep running +# resilient — fall back when config is missing or malformed timeout = config.timeout if config.timeout > 0 else DEFAULT_TIMEOUT - -# defensive, strict — use when a zero timeout means someone messed up -assert config.timeout > 0, "timeout must be positive" ``` -Pick based on whether you want the system to fail or to fall back. Don't do both. +Pick one — fail or fall back — not both. Defensive checks belong where the four-item checklist above doesn't pass. -### 4.6 Use !r Format for Identifiers in Error Messages +### 4.7 Use !r Format for Identifiers in Error Messages **Impact: MEDIUM (produces consistent, unambiguous messages)** @@ -1874,66 +2487,155 @@ raise KeyError(f"unknown key {key!r} in {registry_name!r}") **When backticks are preferable:** some codebases use Markdown-style backticks for user-facing messages (CLI output, log lines humans read). Pick one convention per project and stick to it. `!r` is usually right for Python exception messages; backticks are usually right for log strings rendered in docs or notebooks. -### 4.7 Use assert for Invariants, Not RuntimeError +### 4.8 Use assert Only for Debug-Only Internal Invariants + +**Impact: HIGH (prevents production checks from silently disappearing under -O)** + +`assert` is a **debug-only** statement. The Python language reference is explicit: assertions emit no code when Python is run with `-O` (or `PYTHONOPTIMIZE`), so the check disappears in optimized builds. That makes `assert` the right tool for "this can never happen if my code is correct" — and the *wrong* tool for any check that must run in production. + +**Incorrect (using `assert` to enforce a runtime contract that must hold):** + +```python +def transfer_funds(account_id: str, amount: int) -> None: + assert amount > 0, "amount must be positive" # vanishes under -O + assert account_id, "account_id required" # vanishes under -O + ... +``` + +If this module is imported into a service deployed with `python -O`, both checks compile to nothing. A negative `amount` will sail through and corrupt state; the contract is gone. -**Impact: MEDIUM-HIGH (documents assumptions and fails fast in development)** +**Incorrect (using `assert` for input validation):** + +```python +def parse_request(payload: bytes) -> Request: + data = json.loads(payload) + assert "user_id" in data, "missing user_id" # never trust user input via assert + ... +``` -`assert` documents "this can't happen" — and fails loudly in development if it does. `RuntimeError("internal error")` obscures the intent and fires the same in production, making programming errors look like runtime issues. Reserve exceptions for conditions the caller can reasonably respond to. +User-supplied input must be validated with real exceptions — assertions can be optimized away, and even when present they raise `AssertionError`, which is a poor signal at a system boundary. -**Incorrect (RuntimeError for impossible state):** +**Correct (use `assert` only for "this is impossible if the rest of the code is correct"):** ```python def process_step(step: Step) -> Result: - match step: - case InitStep(): return init() - case RunStep(): return run() - case DoneStep(): return done() + # Step is a closed union; reaching the default branch is a programmer error. + if isinstance(step, InitStep): return init() + if isinstance(step, RunStep): return run() + if isinstance(step, DoneStep): return done() + assert False, f"unhandled Step variant: {step!r}" # debug aid only +``` - raise RuntimeError("unexpected step") # shouldn't be reachable if types are right +The assertion documents the invariant. In development it fires loudly if the union grows a new variant; in production with `-O` it's gone, but at that point you're trusting the type system to have caught the gap. (For exhaustiveness specifically, `typing.assert_never` is sharper — see `error-assert-never-exhaustiveness`.) + +**Correct (use real exceptions for anything that must hold in production):** + +```python +def transfer_funds(account_id: str, amount: int) -> None: + if not account_id: + raise ValueError("account_id required") + if amount <= 0: + raise ValueError("amount must be positive") + ... ``` -If a new `Step` variant is added and this function isn't updated, `RuntimeError("unexpected step")` fires in production. It looks like a runtime problem — but it's a coding error the type system should have caught. +`ValueError` (or a domain-specific exception) is meaningful to callers, can be caught and handled, and survives `-O`. + +**Use `assert` when:** + +- The condition is a programmer-error invariant the type system can't fully express ("this list is sorted," "this counter is non-negative by construction") +- You want a sanity check during development that's free in production +- A `# noqa`-style "I know this can't happen" comment would otherwise be tempting + +**Use a real exception when:** + +- The check guards against caller mistakes (`ValueError`, `TypeError`) +- The input crosses a trust boundary (user input, external API, deserialized data) +- The failure mode is meaningful to the caller (`PermissionError`, `TimeoutError`, custom domain types) +- The check must run in production no matter how the interpreter is invoked -**Correct (assert_never for exhaustiveness; assert for invariants):** +If you can't articulate why losing the check under `-O` is acceptable, it shouldn't be an `assert`. + +### 4.9 Use assert_never for Exhaustiveness Checks + +**Impact: HIGH (turns "missing variant" into a type-check error)** + +`typing.assert_never()` (Python 3.11+) is the right tool for "I've handled every variant of this union." Static checkers treat the call site as unreachable — if the union grows a new member, the checker reports the missed branch as a type error *before* the code ships. At runtime it raises `AssertionError`, so a missed case still fails loudly even if the checker is bypassed. + +This is **separate from** `assert` (the statement). `assert` is debug-only and can be stripped under `-O`; `assert_never` is a function call that always runs and is purpose-built for exhaustiveness narrowing. + +**Incorrect (RuntimeError for unreachable branch — checker doesn't help):** ```python -from typing import assert_never +from dataclasses import dataclass + +@dataclass +class InitStep: ... +@dataclass +class RunStep: ... +@dataclass +class DoneStep: ... + +Step = InitStep | RunStep | DoneStep def process_step(step: Step) -> Result: - match step: - case InitStep(): return init() - case RunStep(): return run() - case DoneStep(): return done() - case _: assert_never(step) # type error at check time if new variant added + if isinstance(step, InitStep): return init() + if isinstance(step, RunStep): return run() + if isinstance(step, DoneStep): return done() + raise RuntimeError(f"unexpected step: {step!r}") # checker can't tell this is exhaustive ``` -`assert_never(step)` is specifically designed for this — the checker will raise a type error if `Step` grows a new variant and the match isn't updated. +If a future change adds `PausedStep` to the union, this function silently falls through to the `RuntimeError` at runtime. The type checker cannot see the gap because `RuntimeError` is not understood as an exhaustiveness assertion. -**Use `assert` for preconditions the checker can't express:** +**Incorrect (plain `assert False` — vanishes under `-O`):** ```python -def binary_search(items: list[int], target: int) -> int: - assert items == sorted(items), "binary_search requires sorted input" - ... +def process_step(step: Step) -> Result: + if isinstance(step, InitStep): return init() + if isinstance(step, RunStep): return run() + if isinstance(step, DoneStep): return done() + assert False, f"unhandled: {step!r}" # stripped under -O; checker doesn't narrow +``` + +Under `python -O`, the assertion compiles to nothing and the function falls off the end with `None` (a worse failure). Type checkers also do not treat plain `assert False` as a guaranteed-unreachable signal in the same way as `assert_never`. + +**Correct (`assert_never` — type error if the union grows, runtime error if reached):** + +```python +from typing import assert_never + +def process_step(step: Step) -> Result: + if isinstance(step, InitStep): return init() + if isinstance(step, RunStep): return run() + if isinstance(step, DoneStep): return done() + assert_never(step) # pyright/mypy: error if Step grows a new variant ``` -This fails fast in development; in production with `-O`, asserts are stripped — which is appropriate because by then the invariant is trusted. +If `Step` later becomes `InitStep | RunStep | DoneStep | PausedStep`, the checker reports that `step` is `PausedStep` at the `assert_never` call — the build breaks before the code ships. -**When to raise an exception instead:** +**Use with `match`/`case` the same way:** + +```python +from typing import assert_never -- The caller could reasonably recover (`FileNotFoundError`, `ValidationError`) -- The input came from an untrusted boundary (user input, external API) -- The failure mode is meaningful to the caller (`PermissionError`, `TimeoutError`) +def process_step(step: Step) -> Result: + match step: + case InitStep(): return init() + case RunStep(): return run() + case DoneStep(): return done() + case _: + assert_never(step) +``` -**When to `assert`:** +**Where `assert_never` belongs:** -- "This can't happen if the rest of the code is correct" -- Internal invariants the checker can't fully enforce -- Sanity checks during development +- Closed sums: `Literal` unions, sealed dataclass hierarchies, discriminated unions +- Enum dispatch where every member must be handled +- Any place where "we covered every case" is a property the checker should enforce -If the condition *can* happen, make it a real exception with a meaningful type. If it genuinely shouldn't happen, `assert` it. +**Backport:** `typing.assert_never` is available from Python 3.11. On older versions, import from `typing_extensions` instead — the semantics are identical and both static checkers recognize either source. -### 4.8 Use raise ... from to Preserve Exception Causality +### 4.10 Use raise ... from to Preserve Exception Causality **Impact: MEDIUM (keeps the original traceback visible for debugging)** @@ -1985,7 +2687,110 @@ The user-facing error is clean (`ValueError: invalid timestamp: 'abc'`) without Default to `from original` when translating between exception types. Reach for `from None` when the internal cause is noise to the caller. -### 4.9 Validate Input at System Boundaries +### 4.11 Use with / async with for Resource Lifetimes + +**Impact: HIGH (deterministic cleanup even on exceptions)** + +Any object that owns a finite resource — file handles, network sockets, database connections, locks, temporary directories, HTTP clients, GPU contexts — should be acquired with `with` (or `async with`). The context-manager protocol guarantees `__exit__` runs even when the body raises, so cleanup happens deterministically. Manual `close()` calls forget to fire on exceptions, leak resources under failure, and are easy to misorder during refactors. + +The same applies to async resources: `async with` exists for `aiohttp` sessions, `httpx.AsyncClient`, `asyncio.Lock`, `anyio` task groups, and async DB drivers. Use it. + +**Incorrect (manual close — leaks on exception):** + +```python +def write_report(path: Path, rows: list[Row]) -> None: + f = path.open("w") + for row in rows: + f.write(format_row(row)) # if this raises, f is never closed + f.close() +``` + +If `format_row` raises midway, `f.close()` never runs. The handle leaks, the file may be left in a partially-written state, and on Windows the path is locked until garbage collection. + +**Incorrect (try/finally — works but verbose; the language gave you `with` for a reason):** + +```python +def write_report(path: Path, rows: list[Row]) -> None: + f = path.open("w") + try: + for row in rows: + f.write(format_row(row)) + finally: + f.close() +``` + +`with` collapses this to one line and removes the chance of forgetting `try`/`finally` next time. + +**Correct (`with` — close runs on success, exception, or early return):** + +```python +def write_report(path: Path, rows: list[Row]) -> None: + with path.open("w") as f: + for row in rows: + f.write(format_row(row)) +``` + +**Resources that must be acquired with a context manager:** + +- **Files:** `open(...)`, `tempfile.TemporaryDirectory()`, `tempfile.NamedTemporaryFile()` +- **Locks:** `threading.Lock()`, `threading.RLock()`, `asyncio.Lock()` +- **Network clients:** `httpx.Client()`, `httpx.AsyncClient()`, `aiohttp.ClientSession()` +- **Database connections / sessions:** `sqlite3.connect()`, SQLAlchemy `Session()`, async DB drivers +- **Subprocess pipes:** `subprocess.Popen` (3.2+ supports `with`) +- **Anything from `contextlib`:** `redirect_stdout`, `suppress`, `chdir` (3.11+), `closing()` + +**Async clients use `async with`:** + +```python +import httpx + +async def fetch_user(user_id: str) -> User: + async with httpx.AsyncClient() as client: + response = await client.get(f"/users/{user_id}") + return User.model_validate_json(response.content) +``` + +`async with` runs `__aexit__` even if `await client.get(...)` raises or the task is cancelled. + +**Stack multiple resources with `contextlib.ExitStack` (or one `with` statement):** + +```python +from contextlib import ExitStack + +def merge_files(inputs: list[Path], output: Path) -> None: + with ExitStack() as stack: + out = stack.enter_context(output.open("w")) + ins = [stack.enter_context(p.open()) for p in inputs] + for src in ins: + for line in src: + out.write(line) +``` + +`ExitStack` closes all resources in reverse order, even if one of the `enter_context` calls raises. + +**Write your own with `@contextmanager`:** + +```python +from contextlib import contextmanager +from collections.abc import Iterator + +@contextmanager +def acquire_lease(resource_id: str) -> Iterator[Lease]: + lease = lease_service.acquire(resource_id) + try: + yield lease + finally: + lease_service.release(lease.id) + +with acquire_lease("worker-42") as lease: + do_work(lease) +``` + +Use the async variant `@contextlib.asynccontextmanager` for resources awaited during acquisition or release. + +**Heuristic:** if you find yourself writing `try` / `finally` to call `close()`, `release()`, `disconnect()`, or `unlink()`, you almost certainly want `with` instead. + +### 4.12 Validate Input at System Boundaries **Impact: HIGH (fails fast and prevents bad data from spreading)** @@ -2377,11 +3182,13 @@ def classify(n: int) -> str: **Rule of thumb:** if you're checking "is this valid?" and returning an error on the no-branch, guard-clause it. If you're splitting between two equal outcomes, `if/else` is fine. -### 5.6 Use @cached_property for Expensive Derived Attributes +### 5.6 Use @cached_property Only When the Instance Supports It + +**Impact: MEDIUM (defers work safely; misuse causes races and silent staleness)** -**Impact: MEDIUM (defers computation and avoids recomputation)** +`@cached_property` is the right tool when the cached value is **derived from effectively immutable inputs**, the getter is **idempotent**, the class **has a writable `__dict__`**, and the instance is not shared across threads racing on first access. Outside that envelope, the convenience masks real bugs: stale caches when inputs mutate, `TypeError` on `__slots__` classes that omit `__dict__`, and duplicated computation when two threads hit the property simultaneously. -When an attribute is computed from other fields, is expensive, and doesn't change over the object's lifetime, `@cached_property` is the right tool. It defers computation until first access and caches the result — avoiding both wasted work when the attribute is never used and repeated work when it's used many times. +The standard library docs are explicit about all of this. Read them once before adding the decorator. **Incorrect (plain method — recomputes on every call):** @@ -2396,7 +3203,7 @@ class Report: return compute_stats(self.rows) ``` -Every call re-walks `self.rows`. If the caller invokes `report.summary_stats()` ten times in a function, you pay ten times. +Every call re-walks `self.rows`. If a caller invokes `report.summary_stats()` ten times, you pay ten times. **Incorrect (eager computation in `__post_init__`):** @@ -2411,40 +3218,78 @@ class Report: You pay at construction time whether or not the caller ever reads `stats`. -**Correct (`@cached_property` — lazy and cached):** +**Incorrect (`@cached_property` on mutable inputs — silent staleness):** ```python from functools import cached_property -from dataclasses import dataclass +from dataclasses import dataclass, field @dataclass class Report: - rows: list[Row] + rows: list[Row] = field(default_factory=list) # mutable; callers can append @cached_property def summary_stats(self) -> Stats: return compute_stats(self.rows) + +r = Report() +r.summary_stats # caches based on empty rows +r.rows.append(new_row) # mutates input +r.summary_stats # still the old cached Stats — stale, no warning ``` -First access runs `compute_stats`; subsequent accesses return the cached result from `self.__dict__`. If no caller reads `summary_stats`, no work happens. +The cache lives in `r.__dict__["summary_stats"]`. Mutating `rows` does not invalidate it. + +**Incorrect (`@cached_property` on a `__slots__` class with no `__dict__`):** + +```python +from functools import cached_property + +class Point: + __slots__ = ("x", "y") # no __dict__ + + def __init__(self, x: int, y: int) -> None: + self.x = x + self.y = y + + @cached_property + def magnitude(self) -> float: + return (self.x ** 2 + self.y ** 2) ** 0.5 -**Caveats:** +Point(3, 4).magnitude +# TypeError: cannot use cached_property instance without the underlying attribute +# (no '__dict__' attribute on 'Point' to cache 'magnitude') +``` -- **Mutability:** if `self.rows` changes after the property is accessed, the cached value is stale. Use `@cached_property` only when the dependencies are effectively immutable. -- **Equality / hashing:** the cache lives in `__dict__`, so it persists across `copy()` unless you clear it. Include `compare=False` on the cache field if using dataclass comparisons. -- **`@property` is still right for cheap derivations** — accessor-like computations (`full_name`, `is_valid`) don't need caching and shouldn't use it. +`cached_property` writes the result into `instance.__dict__`. If the class doesn't have one, the call raises at first access. Either add `"__dict__"` to `__slots__` or use a different caching strategy (`@functools.lru_cache` on a top-level function, an explicit `_cache` field, etc.). -**Use `functools.lru_cache` for module-level pure functions:** +**Correct (lazy and cached, with the inputs effectively immutable):** ```python -from functools import lru_cache +from dataclasses import dataclass, field +from functools import cached_property -@lru_cache(maxsize=256) -def parse_version(s: str) -> Version: - ... +@dataclass(frozen=True) +class Report: + rows: tuple[Row, ...] # immutable container; cannot be mutated after construction + + @cached_property + def summary_stats(self) -> Stats: + return compute_stats(self.rows) ``` -`@cached_property` is the instance-method equivalent. +First access runs `compute_stats`; subsequent accesses return the cached result. Because `rows` is a frozen field of an immutable container, the cache cannot go stale. + +**Caveats to keep in mind:** + +- **Threading:** the docs warn that `cached_property` is not thread-safe. If two threads access the property for the first time at the same time, the getter may run twice. Use a lock, `functools.lru_cache` on a module-level function, or a one-shot `__post_init__` if the work must happen exactly once. +- **Mutability:** if any input the getter reads can change after first access, the cache is wrong. Make the inputs frozen, or stick with a plain method/`@property`. +- **Idempotency:** the getter must produce the same value for the same instance every time. No randomness, no time-dependence, no I/O whose result varies. +- **`__slots__`:** the class must keep `__dict__` available. Slot-only classes need `"__dict__"` in `__slots__`, or skip the decorator. +- **Equality / hashing:** the cached value lands in `__dict__`. Dataclass `eq=True` won't include it (only declared fields), but `copy.copy` carries the cache over — clear it manually if the copy's inputs differ. +- **`@property` is still right for cheap derivations** — accessor-like computations (`full_name`, `is_valid`) don't need caching. + +**For module-level pure functions, use `functools.lru_cache` / `functools.cache` instead** (see `perf-lru-cache-pure-fns`). `@cached_property` is the per-instance equivalent — and only a good fit when the instance meets every condition above. ### 5.7 Use Comprehensions Over for+append Loops @@ -2815,6 +3660,8 @@ Even for the truly-one-shot case, a module-level constant is usually clearer tha **Impact: MEDIUM (avoids repeated schema construction)** +> **Applicability:** this rule is specific to Pydantic v2's `TypeAdapter`. The same principle applies to any object whose constructor does real work (`json.JSONDecoder` with custom hooks, `msgpack.Packer`, compiled templates) — the Pydantic example is the canonical case. + `pydantic.TypeAdapter` does real work on construction — it builds the validation schema for the target type. Inside a hot function, every call rebuilds it. Create it once at module scope and reuse. **Incorrect (rebuilt on every call):** @@ -2873,107 +3720,126 @@ def _adapter_for(model_type: type) -> TypeAdapter: Now each distinct `model_type` gets its adapter built once. -### 6.5 Use Generators for Streaming Iteration +### 6.5 Prefer Tuple Syntax in isinstance() Only on Profiled Hot Paths -**Impact: MEDIUM (constant memory instead of O(n))** +**Impact: LOW (tiny per-call savings; only relevant in tight loops)** -When you're iterating through values and only need them one at a time, a generator uses constant memory. Materializing to a list holds every intermediate value in memory — fine for 100 items, a problem for 100 million. +Both `isinstance(x, (A, B, C))` and `isinstance(x, A | B | C)` are correct and supported in Python 3.10+. They produce the same result. The tuple form is *marginally* faster on each call because the union form constructs a `types.UnionType` object, but the gap is small enough that it only matters inside loops you've actually profiled. **Do not blanket-rewrite a codebase from union to tuple syntax** — the noise is rarely worth the diff. -**Incorrect (materializes a full list just to iterate):** +This is a micro-optimization, not a correctness rule. Apply it only when: -```python -def process_log_lines(path: Path) -> int: - lines = path.read_text().splitlines() # loads entire file - parsed = [parse_line(line) for line in lines] # and a second full list - matching = [p for p in parsed if p.level == "ERROR"] # and a third - return len(matching) -``` +1. The check is inside a measured hot path (a tight loop, called millions of times per request, etc.) +2. You have profiling data showing `isinstance` is a meaningful share of the time +3. You'd otherwise reach for a more invasive change (rewriting the dispatch, caching results) -Three full copies of the data in memory at once. For a 10GB log file, this OOMs. +In normal code, write whichever reads more naturally. `isinstance(x, int | float)` mirrors a type annotation and is a fine default. -**Correct (streaming):** +**Incorrect (rewriting `A | B` to `(A, B)` everywhere as a stylistic crusade):** ```python -def process_log_lines(path: Path) -> int: - with path.open() as f: - count = 0 - for line in f: # iterator over lines - parsed = parse_line(line) - if parsed.level == "ERROR": - count += 1 - return count +# A drive-by PR that flips every isinstance() in the codebase. +def is_numeric(x: object) -> bool: + return isinstance(x, (int, float)) # was: isinstance(x, int | float) ``` -One line at a time. Constant memory regardless of file size. +The diff is pure churn. Annotations elsewhere use `int | float`; the inconsistency makes the codebase harder to read and the savings are imperceptible outside hot paths. -**Generator expressions for pipelines:** +**Correct (apply only on a measured hot path, with a named module-level tuple):** ```python -with path.open() as f: - parsed = (parse_line(line) for line in f) # generator - errors = (p for p in parsed if p.level == "ERROR") # generator - count = sum(1 for _ in errors) # reduces without materializing +# This validator runs once per row across ~10M rows in the ETL job — profiled. +_PRIMITIVE_TYPES = (int, float, str, bool) + +def is_primitive(x: object) -> bool: + return isinstance(x, _PRIMITIVE_TYPES) ``` -Each stage yields one value at a time; nothing is held in memory. +Caching the tuple at module scope and giving it a clear name documents the intent ("this check is hot"). Anywhere else, `isinstance(x, int | float)` is fine. -**When to materialize:** +**Do not rewrite for style alone.** A diff that flips `isinstance(x, A | B)` to `isinstance(x, (A, B))` across a codebase is pure churn — you lose the visual symmetry with type annotations and gain a few microseconds on a path that runs once. -- You need `len()` before iterating (generators don't have a length) -- You iterate the same sequence multiple times (generators exhaust) -- You need random access (`items[5]`) — generators are sequential only -- You need to sort the whole sequence (sort requires materialization anyway) +**Annotations are unaffected.** In type annotations, `X | Y` is the modern form (PEP 604). The tuple form is only relevant inside `isinstance()` / `issubclass()` calls — and only on hot paths. -**`yield` in functions for custom generators:** +### 6.6 Stream with Generators When Memory or First-Result Latency Matters -```python -def read_chunks(path: Path, size: int = 8192) -> Iterator[bytes]: - with path.open("rb") as f: - while chunk := f.read(size): - yield chunk -``` +**Impact: MEDIUM (bounded memory and lazy evaluation for large or infinite sequences)** + +Generators trade materialization for laziness: they yield one value at a time, hold no intermediate list, and let the caller stop early. This is a **memory and streaming** rule, not a "generators are categorically better than lists" rule. When you need every result anyway, iterate the sequence more than once, want random access, or want to sort it — a list comprehension is the right tool, often clearer and sometimes faster (no `next()` overhead per element). + +**Reach for a generator when:** -`yield` builds a generator function — the caller iterates lazily. +- The input is large enough that holding it twice in memory is a problem +- The input is unbounded (infinite stream, line-by-line file read) +- The consumer can stop early (`any()`, `next()`, `break`) +- The pipeline has multiple stages that would otherwise materialize between each -**`itertools` is your friend:** +**Reach for a list (or list comprehension) when:** -`chain`, `islice`, `takewhile`, `dropwhile`, `tee`, `groupby` — all streaming. Use them instead of slicing/filtering materialized lists. +- You need `len()` before iterating +- You iterate the same sequence more than once +- You need random access (`items[5]`) +- You'll sort the whole sequence anyway (sort materializes) +- The data is small and a list comprehension reads more clearly -### 6.6 Use Tuple Syntax in isinstance() Checks +**Incorrect (materializes a multi-GB file for a count — OOMs at scale):** -**Impact: LOW (tuple syntax is measurably faster than union syntax)** +```python +def count_errors(path: Path) -> int: + lines = path.read_text().splitlines() # full file in memory + parsed = [parse_line(line) for line in lines] # second full copy + matching = [p for p in parsed if p.level == "ERROR"] # third full copy + return len(matching) +``` -`isinstance(x, (A, B, C))` and `isinstance(x, A | B | C)` both work. The tuple form is faster at runtime because the union form constructs a `types.UnionType` every call. For hot paths, prefer the tuple. +For a 10GB log file, this OOMs. Three copies of the same data are alive at once. -**Incorrect (union syntax has allocation overhead):** +**Correct (streaming — constant memory regardless of file size):** ```python -def is_primitive(x: object) -> bool: - return isinstance(x, int | float | str | bool) # builds a union type each call +def count_errors(path: Path) -> int: + with path.open() as f: + return sum(1 for line in f if parse_line(line).level == "ERROR") ``` -**Correct (tuple has no per-call overhead):** +One line at a time. Constant memory. + +**Generator expressions for pipelines:** ```python -def is_primitive(x: object) -> bool: - return isinstance(x, (int, float, str, bool)) +with path.open() as f: + parsed = (parse_line(line) for line in f) + errors = (p for p in parsed if p.level == "ERROR") + count = sum(1 for _ in errors) +``` + +Each stage yields one value at a time; nothing is held in memory. + +**Lists are the right call when you'll re-iterate:** + +```python +# Generator would be wrong here — `users` is iterated twice. +users = [u for u in load_users() if u.active] +print(f"{len(users)} active users") +for user in users: + notify(user) ``` -Both forms are semantically equivalent. The tuple version is faster because Python constructs the union type on every call (in older versions) or does extra work comparing against it (in newer versions). +A generator would exhaust on the first iteration and the second loop would run zero times — a real bug, not a performance issue. -**When the difference matters:** +**Lists are also fine when the data is small:** -- Called many times per second in a hot path -- Inside a tight inner loop +```python +# 50 config entries, used once. A generator buys nothing here. +ports = [c.port for c in configs if c.enabled] +``` -**When it doesn't matter:** +Don't replace small list comprehensions with generators on stylistic grounds. -- Called a few times per request -- Rare code paths +**`itertools` for streaming pipelines:** -**Note on annotations vs. runtime checks:** the union syntax (`X | Y`) is idiomatic in type annotations and has zero cost there (annotations aren't evaluated at runtime with `from __future__ import annotations`). The tuple form is only better for the specific case of `isinstance()` calls — other places `X | Y` appears, prefer the union syntax. +`chain`, `islice`, `takewhile`, `dropwhile`, `tee`, `groupby` — all yield lazily. Reach for them when a pipeline is naturally streaming; skip them when the data already fits in memory and a comprehension is clearer. -**Apply consistently** — it's a simple swap, and codebases that use both forms interchangeably make profiling results less predictable. Pick the tuple form once for all `isinstance` checks and move on. +**Heuristic:** ask "what's the worst-case size of this sequence, and does the consumer touch each element exactly once?" If the answer is "large" and "yes," use a generator. Otherwise, write whichever reads more clearly — usually a list comprehension. ### 6.7 Use functools.lru_cache for Pure Functions @@ -3564,7 +4430,118 @@ def export_to_parquet(data: list[dict], path: Path) -> None: Use module-scope for deps the module's public classes require. Use function-scope for features gated behind specific function calls. -### 8.2 No Duplicate Imports +### 8.2 Keep Modules Cheap to Import + +**Impact: MEDIUM (faster CLIs, faster tests, faster worker startup)** + +Importing a module should do as little as possible. Anything at module top-level — opening files, reading environment variables, building large data structures, connecting to databases, registering global handlers, hitting the network — runs every time *anything* in that module is imported. That cost compounds across CLIs (cold-start latency users feel), tests (collection time), worker pools (per-process startup), and serverless functions (cold-start time billed). Heavy import-time side effects also make modules hard to mock and hard to import in the wrong environment. + +Push side effects out of module scope into **functions, factories, lazy properties, or `__init__` methods** that callers invoke explicitly. + +**Incorrect (network call at import time):** + +```python +# config.py +import requests + +CONFIG = requests.get("https://config.example.com/v1").json() # runs on import +DB_URL = CONFIG["db_url"] +``` + +Importing `config` (or anything that imports it transitively) makes a network call. Tests that don't need config still pay. Offline CI breaks. Cold starts add a request worth of latency. + +**Incorrect (heavy initialization at import):** + +```python +# embeddings.py +import torch +from sentence_transformers import SentenceTransformer + +MODEL = SentenceTransformer("all-MiniLM-L6-v2") # 90 MB download + GPU init +``` + +Anyone who imports `embeddings` for a type, a constant, or a single helper triggers a 90 MB download and GPU init. The `--help` of a CLI takes seconds to render. + +**Incorrect (env-dependent failures at import):** + +```python +import os + +API_KEY = os.environ["MY_API_KEY"] # KeyError on import if unset +``` + +Now you cannot import this module to read its docstring without `MY_API_KEY` set. + +**Correct (lazy — pay only when the feature runs):** + +```python +# config.py +from functools import cache +import requests + +@cache +def get_config() -> dict[str, object]: + return requests.get("https://config.example.com/v1").json() + +def db_url() -> str: + return get_config()["db_url"] +``` + +`@cache` makes the first call do the work and subsequent calls hit the cache — same effective behavior as a module constant, but only when something actually asks for it. + +**Correct (lazy — model loaded on first use):** + +```python +# embeddings.py +from functools import cache + +@cache +def get_model() -> "SentenceTransformer": + from sentence_transformers import SentenceTransformer + return SentenceTransformer("all-MiniLM-L6-v2") + +def embed(text: str) -> list[float]: + return get_model().encode(text).tolist() +``` + +The heavy import lives inside the function (see `imports-top-of-file` for when inline imports are okay). The model loads on first `embed`, not on `import`. + +**Correct (env read at use, with a clear error):** + +```python +import os + +def api_key() -> str: + key = os.environ.get("MY_API_KEY") + if not key: + raise RuntimeError("MY_API_KEY is required to call this API") + return key +``` + +Now the module imports anywhere, and the missing env variable surfaces with a useful message at the call site. + +**What is fine to do at import time:** + +- Pure-Python constants: `MAX_RETRIES = 5`, `_NAME_RE = re.compile(r"...")` (compile is cheap and amortizes) +- Class and function definitions +- Cheap, deterministic, in-process work (registering a dataclass, creating a small lookup dict) +- Standard-library imports + +**What to push out of import time:** + +- Network or disk I/O +- Subprocess launches +- Loading large models, datasets, ML weights +- Reading environment variables that may be missing +- Connecting to databases or message queues +- Registering signal handlers, atexit hooks, observability sinks +- Heavy third-party imports the module doesn't use unconditionally + +**Test for it.** Run `python -c "import yourpackage"` with `--time` or in `cProfile`. If a single import takes more than ~100 ms or makes any network call, find what's running at module scope and defer it. + +**Heuristic:** if the work needs to happen *exactly once* per process, write a `@cache`-decorated function and call it on demand. If it needs to happen *every* call, write a regular function. Module-scope side effects are almost never the right choice — they're "every import" by accident, not "once per process" by design. + +### 8.3 No Duplicate Imports **Impact: LOW (prevents confusion and redundant work)** @@ -3622,7 +4599,7 @@ Add to pre-commit or CI. Reviewing the imports block after any merge or mass edit catches these before they land. -### 8.3 Place Imports at the Top of the File +### 8.4 Place Imports at the Top of the File **Impact: LOW-MEDIUM (makes dependencies visible at a glance)** @@ -3698,7 +4675,7 @@ This is the narrow exception — think twice before using it. See `imports-optio Outside these cases, top-of-file is the rule. -### 8.4 Remove Unused Imports +### 8.5 Remove Unused Imports **Impact: LOW-MEDIUM (prevents accidental dependencies and reduces noise)** @@ -3767,7 +4744,7 @@ If an import is used only in annotations, move it under `if TYPE_CHECKING:` (see Outside those cases: delete. -### 8.5 Scope Helpers and Constants to Their Usage Site +### 8.6 Scope Helpers and Constants to Their Usage Site **Impact: LOW-MEDIUM (reduces namespace pollution and clarifies intent)** @@ -3831,8 +4808,17 @@ def summarize(text: str) -> str: ## References - https://docs.python.org/3/library/typing.html +- https://docs.python.org/3/library/dataclasses.html +- https://docs.python.org/3/library/exceptions.html +- https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement - https://docs.pydantic.dev/ - https://mypy.readthedocs.io/ +- https://docs.astral.sh/ruff/ +- https://peps.python.org/pep-0008/ - https://peps.python.org/pep-0544/ +- https://peps.python.org/pep-0604/ +- https://peps.python.org/pep-0615/ +- https://peps.python.org/pep-0661/ - https://peps.python.org/pep-0695/ +- https://peps.python.org/pep-0702/ - https://github.com/pydantic/pydantic-ai diff --git a/skills/python-best-practices/README.md b/skills/python-best-practices/README.md index b52fb74..6ade904 100644 --- a/skills/python-best-practices/README.md +++ b/skills/python-best-practices/README.md @@ -2,21 +2,32 @@ A structured skill for writing and reviewing Python code. Rules are derived from real PR review patterns, organized by impact, and formatted for AI-agent consumption. +**Python version baseline:** 3.11+ (some rules note higher-version features explicitly — e.g., `warnings.deprecated()` is 3.13+). + ## Structure -- `rules/` — Individual rule files (one per rule) - - `_sections.md` — Section metadata (titles, impacts, descriptions, prefixes) - - `_template.md` — Template for creating new rules - - `{prefix}-{name}.md` — Individual rule files -- `SKILL.md` — Entrypoint loaded into agent context -- `AGENTS.md` — Compiled document with all rules expanded -- `metadata.json` — Version and abstract +``` +python-best-practices/ +├── SKILL.md # Entrypoint loaded into agent context (quick reference) +├── README.md # This file — human-facing overview and contribution notes +├── metadata.json # Version, abstract, references, Python version floor +├── AGENTS.md # (generated) Compiled document with every rule expanded +├── test-cases.json # (generated) LLM evaluation data extracted from rule examples +├── rules/ # Individual rule files (one rule per file) +│ ├── _sections.md # Section metadata (titles, impacts, descriptions, prefixes) +│ ├── _template.md # Template for new rules (with required `references` line) +│ └── {prefix}-{name}.md # Rule files; `prefix` matches a section in `_sections.md` +└── src/ # Build, validate, and extract-tests scripts + ├── build.py # Compile rules into AGENTS.md + ├── validate.py # Lint rule files (frontmatter, examples, references) + └── extract_tests.py # Generate test-cases.json from rule examples +``` ## Sections ### 1. Data Modeling (CRITICAL) — `data-` -Derive over store, discriminated unions, explicit variants, mutation contracts. The architectural foundation — mistakes here compound hardest. +Derive over store, discriminated unions, explicit variants, mutation contracts, mutable defaults, sentinels, timezone-aware datetimes. The architectural foundation — mistakes here compound hardest. ### 2. Type Safety (CRITICAL) — `types-` @@ -24,11 +35,11 @@ No `Any` drift, precise annotations, proper narrowing. The type checker is load- ### 3. API Design (HIGH) — `api-` -Keyword-only params, private underscores, immutable transforms. Interface decisions that compound over years. +Keyword-only params, private underscores, immutable transforms, no boolean flag soup. Interface decisions that compound over years. ### 4. Error Handling (HIGH) — `error-` -Specific exceptions, fail-fast validation, consolidated try/except. Sloppy exceptions hide bugs; good ones localize them. +Specific exceptions, fail-fast validation, consolidated try/except, context managers for resources, exhaustiveness via `assert_never`. Sloppy exceptions hide bugs; good ones localize them. ### 5. Code Simplification (MEDIUM-HIGH) — `simplify-` @@ -36,7 +47,7 @@ Comprehensions, `any()`/`all()`, early returns, dead-code removal. Python idioms ### 6. Performance (MEDIUM) — `perf-` -Module-level compilation, set/dict lookups, cached properties. Python-specific optimizations that matter on hot paths. +Module-level compilation, set/dict lookups, cached properties. Python-specific optimizations applied where the hot path is measured. ### 7. Naming (MEDIUM) — `naming-` @@ -44,46 +55,80 @@ Specific names, consistent terminology, no type suffixes. Names are the most-rea ### 8. Imports & Structure (LOW-MEDIUM) — `imports-` -Top-of-file imports, optional dependency handling. Module hygiene. +Top-of-file imports (with documented exceptions), optional dependency handling, no import-time side effects. Module hygiene. -## Creating a New Rule +## Authoring Workflow 1. Copy `rules/_template.md` to `rules/{prefix}-{name}.md` 2. Choose the appropriate prefix from `_sections.md` -3. Fill in the frontmatter and content -4. Ensure you have clear incorrect/correct examples with explanations +3. Fill in the frontmatter (including a primary-source `references` line for any rule that depends on language version or library behavior) +4. Write a short explanation, an Incorrect/Correct pair, and a closing note about edge cases +5. Run the build / validate / extract-tests scripts (below) + +## Scripts + +The `src/` directory contains the maintenance pipeline: + +```bash +# Compile rules into AGENTS.md +python src/build.py + +# Lint rule files (frontmatter, references, example structure, broken links) +python src/validate.py + +# Extract Incorrect/Correct example pairs into test-cases.json (for LLM evals) +python src/extract_tests.py +``` + +A typical authoring loop is `validate.py` → fix → `build.py` → `extract_tests.py` before committing. `AGENTS.md` and `test-cases.json` are generated outputs — do not edit them by hand. ## Rule File Format -Each rule file should follow this structure: +Each rule file follows this structure: ```markdown --- title: Rule Title Here impact: MEDIUM impactDescription: brief phrase describing the payoff -tags: tag1, tag2 +tags: tag1, tag2, applicability:pydantic # `applicability:` for ecosystem-specific rules +references: https://docs.python.org/3/library/... --- ## Rule Title Here -Brief explanation of the rule and why it matters. One or two sentences. +Brief explanation of the rule and why it matters. Name the impulse the agent is tempted to take. -**Incorrect (why this is wrong):** +**Incorrect (what's wrong with this):** -\`\`\`python +```python # Bad example -\`\`\` +``` -**Correct (why this is right):** +**Correct (what's right about this):** -\`\`\`python +```python # Good example -\`\`\` +``` -Optional closing paragraph with nuance or references. +Optional closing paragraph with nuance, edge cases, or version notes. ``` +### When `references` is required + +`references` is **required** when the rule depends on: + +- A specific Python version (3.10 union types in `isinstance`, 3.11 `assert_never`, 3.13 `warnings.deprecated`) +- Standard-library behavior (`assert` under `-O`, `cached_property` thread safety) +- Third-party library behavior (Pydantic, mypy, ruff) +- A PEP + +Pure judgment-call rules (naming preferences, taste) may omit `references` but adding one is encouraged. + +### Tagging applicability + +Rules that only apply within a specific ecosystem (e.g., Pydantic) carry an `applicability:{name}` tag and call it out in the body. This lets future filtering/eval pipelines skip rules that don't apply to a given codebase. + ## Impact Levels - `CRITICAL` — Highest priority; prevents classes of bugs or unmaintainable code @@ -91,7 +136,7 @@ Optional closing paragraph with nuance or references. - `MEDIUM-HIGH` — Noticeable improvements worth enforcing - `MEDIUM` — Good practices for cleaner, clearer code - `LOW-MEDIUM` — Marginal improvements -- `LOW` — Incremental; apply opportunistically +- `LOW` — Incremental; apply opportunistically (e.g., micro-optimizations on profiled hot paths only) ## Acknowledgments diff --git a/skills/python-best-practices/SKILL.md b/skills/python-best-practices/SKILL.md index e25ad59..7b47636 100644 --- a/skills/python-best-practices/SKILL.md +++ b/skills/python-best-practices/SKILL.md @@ -4,14 +4,15 @@ description: Python software engineering guidelines from real PR review patterns license: MIT metadata: author: python-best-practices - version: "1.0.0" + version: "1.1.0" + pythonVersion: ">=3.11" --- # Python Best Practices -Comprehensive guidelines for Python codebases that must resist drift over time. Contains 50+ rules across 8 categories, prioritized by impact to guide automated refactoring and code generation. +Comprehensive guidelines for Python codebases that must resist drift over time. 60+ rules across 8 categories, prioritized by impact to guide automated refactoring and code generation. -The rules codify the failure modes agents fall into: reaching for `Any`, stacking optional fields into grab-bag models, catching bare `Exception`, and bypassing type checkers with `# type: ignore`. Each rule names the impulse, shows the failure, and points at the better path. +The rules codify the failure modes agents fall into: reaching for `Any`, stacking optional fields into grab-bag models, catching bare `except:`, using mutable defaults, and bypassing type checkers with `# type: ignore`. Each rule names the impulse, shows the failure, and points at the better path. ## When to Apply @@ -24,6 +25,17 @@ Reference these guidelines when: - Adding exception handling, validation, or error paths - Optimizing hot paths for performance +## Python Version Baseline + +Rules assume **Python 3.11+** as the floor (for `assert_never`, `Self`, exception groups, `tomllib`). Rules that depend on a higher version call it out inline: + +- `warnings.deprecated()` — 3.13+ +- `zoneinfo` — 3.9+ +- Union types in `isinstance()` — 3.10+ +- `assert_never` — 3.11+ (use `typing_extensions` to backport) + +Rules tagged `applicability:pydantic` are Pydantic-specific. + ## Rule Categories by Priority | Priority | Category | Impact | Prefix | @@ -46,7 +58,10 @@ Reference these guidelines when: - `data-explicit-variants` — Concrete classes per mode beat one class with `is_thread`/`is_edit`/`is_forward` flags - `data-phased-composition` — Group co-present optional fields into one nested optional, not eight siblings - `data-mutation-contract` — Mutate OR return; never both (callers can't tell which to use) -- `data-encapsulate-mutable-state` — Trap mutable state in the smallest possible scope +- `data-encapsulate-mutable-state` — Trap mutable state in the **narrowest clear scope** — closure, focused class, or instance attribute as the case demands +- `data-mutable-defaults` — Never `def f(items=[])`; use `None` + body construction or `default_factory` +- `data-sentinel-when-none-is-valid` — Use a private sentinel when `None` is itself a meaningful domain value +- `data-aware-datetimes` — Timezone-aware `datetime.now(timezone.utc)` at every boundary; `datetime.utcnow()` is deprecated - `data-delete-dead-variants` — Remove union/enum branches that are never constructed - `data-newtype-for-ids` — Brand primitive IDs (`NewType('UserId', str)`) so they aren't interchangeable @@ -67,23 +82,27 @@ Reference these guidelines when: - `api-required-before-optional` — Required fields before optional in dataclasses (Python enforces this) - `api-keyword-only-params` — `*` or `KW_ONLY` marker for optional/config params to prevent breakage +- `api-no-boolean-flag-params` — `Literal`/`Enum` over positional `True, False` soup; split functions when bodies barely overlap - `api-underscore-for-private` — `_prefix` for internals; exclude from `__all__` - `api-immutable-transforms` — Return new collections; don't mutate inputs (unless named `update_*` / `*_inplace`) -- `api-deprecated-aliases` — Old names stay as aliases when renaming public API +- `api-deprecated-aliases` — `warnings.deprecated()` (3.13+) for renamed funcs/classes; compatibility kwargs + `warnings.warn` for renamed parameters - `api-no-private-access` — Don't reach into `_prefixed` names from outside the module - `api-model-cohesion` — Keep models flat; avoid duplicate/single-key-wrapped/redundant fields -- `api-instance-vs-module-fn` — Instance methods for stateful behavior; module-level fns for pure utilities +- `api-instance-vs-module-fn` — Pick the simplest namespace that matches ownership and polymorphism ### 4. Error Handling (HIGH) -- `error-specific-exceptions` — Catch specific types; never bare `except Exception` +- `error-no-bare-except` — `except:` catches `KeyboardInterrupt`/`SystemExit`/`CancelledError`; never use it +- `error-specific-exceptions` — Catch specific types; `except Exception:` only at outer-loop log-and-reraise sites +- `error-context-managers` — `with` / `async with` for files, locks, sessions, temp dirs; not manual `close()` - `error-consolidate-try-except` — Merge blocks that catch the same exception with similar handling -- `error-assert-invariants` — `assert` for invariants that can't fail; not `RuntimeError('internal error')` +- `error-assert-debug-only` — `assert` is stripped under `-O`; only use it for debug-only invariants +- `error-assert-never-exhaustiveness` — `typing.assert_never` for exhaustiveness checks (3.11+) - `error-validate-at-boundaries` — Validate input before expensive work; fail fast at system edges - `error-inherit-base-exceptions` — New exceptions inherit from existing bases for backward compatibility - `error-repr-in-messages` — `f"tool {name!r}"` for identifiers in error text; consistent quoting - `error-raise-from-for-chains` — `raise NewErr(...) from original` to preserve causality -- `error-trust-validated-state` — No defensive re-checks after earlier validation; trust the invariant +- `error-trust-validated-state` — Trust validated, immutable, locally-constructed state in the same trust domain; keep checks for mutable/external/rehydrated objects - `error-preserve-cancellation` — `CancelledError` is `BaseException` on 3.8+; don't false-flag `except Exception:` for "swallowing cancellation" ### 5. Code Simplification (MEDIUM-HIGH) @@ -93,7 +112,7 @@ Reference these guidelines when: - `simplify-fallback-or` — `x or default` over verbose `if`/`else` (when falsy values aren't semantic) - `simplify-inline-single-use-vars` — Drop `_filtered`, `_copy` intermediates that are used once - `simplify-flatten-nested-if` — Combine into `if cond1 and cond2:` when there's no intervening code -- `simplify-cached-property` — `@cached_property` for expensive derived attributes +- `simplify-cached-property` — `@cached_property` for derived attrs on **immutable** instances with `__dict__`; not thread-safe - `simplify-extract-after-duplication` — Extract helpers once a pattern repeats; don't copy-paste a third time - `simplify-remove-dead-code` — Delete commented-out code and unused definitions; git preserves history - `simplify-early-return` — Return early; don't nest the happy path three levels deep @@ -101,12 +120,12 @@ Reference these guidelines when: ### 6. Performance (MEDIUM) - `perf-compile-regex-module-level` — Compile static regex at module scope; not inside hot functions -- `perf-type-adapter-constant` — Define `TypeAdapter` instances at module scope +- `perf-type-adapter-constant` — Define Pydantic `TypeAdapter` instances at module scope *(applicability: pydantic)* - `perf-set-for-membership` — `set` for repeated `in` checks; O(1) beats `list.__contains__` - `perf-dict-index-over-nested-loops` — Build a `dict` for lookups; not nested `for` + `if` -- `perf-generator-over-list` — Generators for streaming iteration; materialize only when needed +- `perf-generator-over-list` — Stream with generators when memory or first-result latency matters; lists are fine when you re-iterate or need `len()` - `perf-lru-cache-pure-fns` — `functools.lru_cache` / `functools.cache` for pure functions -- `perf-isinstance-tuple-syntax` — `isinstance(x, (A, B))` over `isinstance(x, A | B)` (tuple is faster) +- `perf-isinstance-tuple-syntax` — Tuple form is marginally faster; **only rewrite on profiled hot paths**, not as a stylistic crusade - `perf-combine-iterations` — Fuse `filter` + `map` into one pass when possible ### 7. Naming (MEDIUM) @@ -120,7 +139,8 @@ Reference these guidelines when: ### 8. Imports & Structure (LOW-MEDIUM) -- `imports-top-of-file` — All imports at the top; not inline in function bodies +- `imports-top-of-file` — Imports at the top by default; documented exceptions for circular imports, optional heavy deps, and side-effect deferral +- `imports-no-side-effects` — Modules must be cheap to import — no network calls, model loads, env reads, or registrations at import time - `imports-optional-dependencies` — `try`/`except ImportError` with helpful install hints - `imports-remove-unused` — Delete unused imports; keep module namespace tight - `imports-no-duplicates` — One import per name @@ -140,8 +160,19 @@ Each rule file contains: - Brief explanation of why it matters - Incorrect code example with explanation - Correct code example with explanation -- Additional context, nuance, or references +- Primary-source `references` (PEPs, stdlib docs, library docs) when version- or library-dependent +- Closing notes on edge cases or applicability + +## Authoring & Maintenance + +The skill ships with a build/validate/extract-tests pipeline (see `README.md`): + +- `python src/build.py` — compile rules into `AGENTS.md` +- `python src/validate.py` — lint frontmatter, references, and example structure +- `python src/extract_tests.py` — generate `test-cases.json` for LLM evals + +Rule files live under `rules/`; `AGENTS.md` and `test-cases.json` are generated outputs. ## Full Compiled Document -For the complete guide with every rule expanded: `AGENTS.md` +For the complete guide with every rule expanded: `AGENTS.md`. Note that `AGENTS.md` is large by design (every rule body) — agents can grep individual rule files in `rules/` instead when only one or two rules are relevant. diff --git a/skills/python-best-practices/metadata.json b/skills/python-best-practices/metadata.json index bb54df2..c8ab1fb 100644 --- a/skills/python-best-practices/metadata.json +++ b/skills/python-best-practices/metadata.json @@ -1,14 +1,24 @@ { - "version": "1.0.0", + "version": "1.1.0", "organization": "Python Best Practices", "date": "April 2026", - "abstract": "Comprehensive Python software engineering guidelines designed for AI agents. Contains 50+ rules across 8 categories, prioritized by impact from critical (data modeling, type safety) to low (import hygiene). Each rule names the failure mode agents tend toward, shows incorrect and correct code, and explains the payoff. Rules are derived from real PR review patterns and production experience.", + "pythonVersion": ">=3.11", + "abstract": "Comprehensive Python software engineering guidelines designed for AI agents. 60+ rules across 8 categories, prioritized by impact from critical (data modeling, type safety) to low (import hygiene). Each rule names the failure mode agents tend toward, shows incorrect and correct code, cites primary-source references where the rule depends on language or library behavior, and explains the payoff. Rules assume Python 3.11+ as a baseline; rules that depend on a higher version (e.g., 3.13 for warnings.deprecated) are tagged accordingly.", "references": [ "https://docs.python.org/3/library/typing.html", + "https://docs.python.org/3/library/dataclasses.html", + "https://docs.python.org/3/library/exceptions.html", + "https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement", "https://docs.pydantic.dev/", "https://mypy.readthedocs.io/", + "https://docs.astral.sh/ruff/", + "https://peps.python.org/pep-0008/", "https://peps.python.org/pep-0544/", + "https://peps.python.org/pep-0604/", + "https://peps.python.org/pep-0615/", + "https://peps.python.org/pep-0661/", "https://peps.python.org/pep-0695/", + "https://peps.python.org/pep-0702/", "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/pydantic/pydantic-ai" ] } diff --git a/skills/python-best-practices/rules/_template.md b/skills/python-best-practices/rules/_template.md index ccfbfd0..0dd6700 100644 --- a/skills/python-best-practices/rules/_template.md +++ b/skills/python-best-practices/rules/_template.md @@ -3,6 +3,7 @@ title: Rule Title Here impact: MEDIUM impactDescription: brief phrase describing the payoff (e.g., "prevents drift between call sites") tags: tag1, tag2 +references: https://primary-source-1.example.com, https://primary-source-2.example.com --- ## Rule Title Here @@ -22,3 +23,23 @@ Brief explanation of the rule and why it matters. One or two sentences. Name the ``` Optional closing paragraph with nuance, edge cases, or references. + +--- + +## Authoring notes + +**The `references` field is required when the rule depends on language-version or library behavior.** Link to primary sources — the language reference, library docs, or a PEP. If the rule is a pure judgment call (e.g., a naming preference), `references` may be omitted, but adding one is still encouraged. + +Examples of when references are required: + +- Rule mentions a specific Python version (e.g., 3.10, 3.11, 3.13) +- Rule cites stdlib behavior (e.g., `assert` semantics under `-O`, `cached_property` thread safety) +- Rule cites third-party library behavior (Pydantic, mypy, ruff) +- Rule cites a PEP + +Examples of when references may be omitted: + +- "Use `_prefix` for private helpers" (project convention, no version dependency) +- "Specific names beat generic names" (taste / readability) + +Keep references to **primary sources**. Blog posts and tutorials drift; PEPs and stdlib docs do not. diff --git a/skills/python-best-practices/rules/api-deprecated-aliases.md b/skills/python-best-practices/rules/api-deprecated-aliases.md index 0df91eb..252aac4 100644 --- a/skills/python-best-practices/rules/api-deprecated-aliases.md +++ b/skills/python-best-practices/rules/api-deprecated-aliases.md @@ -3,6 +3,7 @@ title: Keep Old Names as Deprecated Aliases impact: HIGH impactDescription: enables gradual migration without breakage tags: api, deprecation, compatibility +references: https://peps.python.org/pep-0702/, https://docs.python.org/3/library/warnings.html#warnings.deprecated, https://docs.python.org/3/library/warnings.html#warnings.warn --- ## Keep Old Names as Deprecated Aliases @@ -19,7 +20,7 @@ def get_user(user_id: str) -> User: ... def fetch_user(user_id: str) -> User: ... # renamed — v1.0 callers now crash ``` -**Correct (deprecated alias):** +**Correct (deprecated alias with `warnings.warn`):** ```python import warnings @@ -38,29 +39,55 @@ def get_user(user_id: str) -> User: Old callers keep working with a warning; new callers use the new name. -**For parameter renames, use `typing.deprecated` (Python 3.13+) or keyword handling:** +**On Python 3.13+, prefer `warnings.deprecated()` for whole functions, classes, and overloads.** PEP 702 added a standard decorator that emits the warning, marks the symbol so static checkers can flag callers, and surfaces the deprecation in IDE tooling. The decorator lives in `warnings`, **not** `typing`: ```python +import warnings # Python 3.13+ + +@warnings.deprecated("get_user is deprecated; use fetch_user instead.") +def get_user(user_id: str) -> User: + return fetch_user(user_id) + + +@warnings.deprecated("LegacyClient is deprecated; use Client instead.") +class LegacyClient(Client): ... +``` + +Type checkers that implement PEP 702 (mypy, pyright) report calls to deprecated names without you having to wire `warnings.warn` by hand. + +**For renamed parameters, `warnings.deprecated()` does not apply** — it decorates whole symbols, not individual parameters. Use a compatibility keyword path plus a runtime warning inside the function: + +```python +import warnings +from typing import Any + +_MISSING: Any = object() + def fetch_user( - user_id: str, + user_id: str = _MISSING, *, timeout: float = 30, - user_id_alt: str | None = None, # old name + user_id_alt: str = _MISSING, # old name; remove in next major ) -> User: - if user_id_alt is not None: + if user_id_alt is not _MISSING: warnings.warn( - "user_id_alt is deprecated; pass user_id instead.", + "the user_id_alt parameter is deprecated; pass user_id instead.", DeprecationWarning, stacklevel=2, ) - user_id = user_id_alt + if user_id is _MISSING: + user_id = user_id_alt + if user_id is _MISSING: + raise TypeError("fetch_user() missing required argument: 'user_id'") ... ``` +The compatibility shim (the old keyword still accepted, then forwarded) is what preserves callers; the `warnings.warn(..., DeprecationWarning, stacklevel=2)` call is what surfaces the migration. + **Deprecation policy:** 1. Add the new name. Old name becomes an alias. -2. Emit a `DeprecationWarning` explaining the migration. +2. Emit a `DeprecationWarning` (via `warnings.warn` or `@warnings.deprecated`) explaining the migration. 3. Document the deprecation in the changelog and docstrings. 4. Remove the alias in a later major version (follow your project's deprecation window — typically one or two releases). diff --git a/skills/python-best-practices/rules/api-instance-vs-module-fn.md b/skills/python-best-practices/rules/api-instance-vs-module-fn.md index 8fa4567..73ed7ba 100644 --- a/skills/python-best-practices/rules/api-instance-vs-module-fn.md +++ b/skills/python-best-practices/rules/api-instance-vs-module-fn.md @@ -1,25 +1,26 @@ --- -title: Instance Methods for State, Module Functions for Pure Logic +title: Choose the Simplest Namespace That Matches Ownership and Polymorphism impact: MEDIUM -impactDescription: avoids unnecessary coupling while enabling polymorphism -tags: api, methods, functions, design +impactDescription: avoids unnecessary coupling without forcing a binary choice +tags: api, methods, functions, design, namespace +references: https://docs.python.org/3/tutorial/classes.html --- -## Instance Methods for State, Module Functions for Pure Logic +## Choose the Simplest Namespace That Matches Ownership and Polymorphism -Agents tend to put everything on classes because "that's how OOP works" — or conversely, make everything a module-level function because "pure is better." The right call depends on whether the function genuinely needs `self` or enables polymorphism. +Python lets the same logic live as a module-level function, an instance method, a `@classmethod`, a `@staticmethod`, a method on a `Protocol`, or a method on a `dataclass`. None of these is universally right. Pick the smallest namespace that captures **ownership** (does this operation belong to one object?) and **polymorphism** (will multiple types provide their own version?). -**Use an instance method when:** -- The function accesses `self` attributes -- It's a natural operation on the object (the method *is* part of the object's interface) -- Subclasses will override it (polymorphism) +A useful decision order, from simplest to most coupled: -**Use a module-level function when:** -- Nothing about the logic depends on instance state -- The function is a pure utility that happens to take an object of that class -- Multiple classes could reasonably use the same helper +1. **Module-level function** — when the logic is a pure utility that operates on its arguments and doesn't need to be overridden. +2. **Instance method** — when the logic naturally reads as "this object does X" and uses `self`, *or* when subclasses / Protocol implementations need to provide their own version. +3. **`@classmethod`** — alternative constructors, factory methods, things that need the class but not an instance. +4. **`@staticmethod`** — namespace grouping when the helper is conceptually tied to the class but takes no `self`/`cls`. Often a sign a module-level function would do. +5. **Protocol** — when several unrelated types need to provide the same interface and you want structural typing. -**Incorrect (module-level function awkwardly threading state):** +There is no "correct" tier; pick the simplest one that fits. + +**Incorrect (module-level function awkwardly threading state through `user`):** ```python def update_user_preferences(user: User, key: str, value: object) -> None: @@ -30,9 +31,9 @@ def get_user_display_name(user: User) -> str: return f"{user.first_name} {user.last_name}" ``` -These both mutate/read `user` state and are core user operations — they belong on `User`. +These mutate or read `user` state, name `user` in their parameter list, and have no second caller type. They belong on `User`. -**Correct (instance methods):** +**Correct (instance methods — ownership matches the object):** ```python class User: @@ -45,12 +46,12 @@ class User: return f"{self.first_name} {self.last_name}" ``` -**Incorrect (instance method that doesn't need `self`):** +**Incorrect (instance method that doesn't need `self` and isn't overridden):** ```python class DateFormatter: def format_iso(self, d: date) -> str: - return d.isoformat() # doesn't touch self + return d.isoformat() # `self` is unused ``` **Correct (module-level function):** @@ -60,6 +61,38 @@ def format_iso(d: date) -> str: return d.isoformat() ``` -**Extract shared logic to private top-level helpers** when multiple classes need the same computation — don't duplicate it across methods. +If five subclasses of `DateFormatter` are about to override `format_iso` with locale-specific behavior, the method form is correct after all — polymorphism justifies the coupling. + +**`@classmethod` for alternative constructors:** + +```python +class Event: + def __init__(self, kind: str, payload: dict[str, Any]) -> None: + self.kind = kind + self.payload = payload + + @classmethod + def from_json(cls, raw: str) -> "Event": + data = json.loads(raw) + return cls(kind=data["kind"], payload=data["payload"]) +``` + +`from_json` doesn't need an instance, but it does need the class for subclass-friendly construction. + +**`@staticmethod` is the rarest tier.** If the function takes no `self` and no `cls`, the only reason to attach it to a class is namespacing — and a module-level function is usually cleaner. Reserve `@staticmethod` for cases where the class genuinely makes the helper more discoverable (a small private validator on a model, for example). + +**Protocols when the consumer doesn't need to know the producer:** + +```python +from typing import Protocol + +class JSONSerializable(Protocol): + def to_json(self) -> str: ... + +def write(obj: JSONSerializable, path: Path) -> None: + path.write_text(obj.to_json()) +``` + +Now any type with `to_json` works — no shared base class, no inheritance. -**`@staticmethod` / `@classmethod`:** reach for these sparingly. If a method doesn't need `self` or `cls`, it's usually a module-level function. Reserve them for alternative constructors (`@classmethod`) or namespace-grouped utilities where the class genuinely makes things more discoverable. +**Heuristic:** start at module scope. Promote to a method only when ownership or polymorphism *actually* demand it. The cost of starting too coupled (everything on a class) is harder to undo than the cost of starting too loose (a free function you later move). diff --git a/skills/python-best-practices/rules/api-keyword-only-params.md b/skills/python-best-practices/rules/api-keyword-only-params.md index 4c1a982..2da631f 100644 --- a/skills/python-best-practices/rules/api-keyword-only-params.md +++ b/skills/python-best-practices/rules/api-keyword-only-params.md @@ -3,6 +3,7 @@ title: Use Keyword-Only Parameters for Optional Config impact: HIGH impactDescription: prevents breakage when adding or reordering params tags: api, parameters, keyword-only, compatibility +references: https://docs.python.org/3/library/dataclasses.html#dataclasses.KW_ONLY, https://peps.python.org/pep-3102/ --- ## Use Keyword-Only Parameters for Optional Config diff --git a/skills/python-best-practices/rules/api-no-boolean-flag-params.md b/skills/python-best-practices/rules/api-no-boolean-flag-params.md new file mode 100644 index 0000000..fb50cff --- /dev/null +++ b/skills/python-best-practices/rules/api-no-boolean-flag-params.md @@ -0,0 +1,100 @@ +--- +title: Avoid Boolean Flag Parameters in Public APIs +impact: HIGH +impactDescription: prevents call sites that read like "do_thing(thing, True, False)" +tags: api, parameters, booleans, literal, enum +references: https://docs.python.org/3/library/typing.html#typing.Literal, https://docs.python.org/3/library/enum.html +--- + +## Avoid Boolean Flag Parameters in Public APIs + +A boolean parameter is a binary mode switch hiding behind a generic type. The call site `download(url, True, False, True)` is unreadable, the function body branches on the flag with two near-duplicate code paths, and adding a third mode later requires breaking the API. This is the function-level cousin of `data-explicit-variants`: when behavior meaningfully changes on a flag, prefer split functions or a `Literal`/`Enum` parameter. + +**Incorrect (boolean flags — call sites lose meaning):** + +```python +def export_report(rows: list[Row], to_csv: bool = True, compress: bool = False) -> bytes: + if to_csv: + data = render_csv(rows) + else: + data = render_json(rows) + if compress: + data = gzip.compress(data) + return data + +export_report(rows, True, False) # what does True/False mean here? +export_report(rows, False, True) # JSON, compressed? CSV, compressed? Reader can't tell. +``` + +The function body is two if-branches stacked, the call sites carry no information, and any third format (Parquet, XML) means another bool — `to_csv: bool, to_json: bool, to_parquet: bool` is incoherent. + +**Correct option A (split into separate functions when bodies barely overlap):** + +```python +def export_csv(rows: list[Row]) -> bytes: ... +def export_json(rows: list[Row]) -> bytes: ... + +def with_compression(data: bytes) -> bytes: + return gzip.compress(data) + +# call site +data = with_compression(export_csv(rows)) +``` + +Each function does one thing. Adding `export_parquet` is additive, not breaking. Compression composes orthogonally. + +**Correct option B (`Literal` parameter when the modes share most of the body):** + +```python +from typing import Literal + +Format = Literal["csv", "json", "parquet"] + +def export_report(rows: list[Row], format: Format, *, compress: bool = False) -> bytes: + match format: + case "csv": data = render_csv(rows) + case "json": data = render_json(rows) + case "parquet": data = render_parquet(rows) + return gzip.compress(data) if compress else data + +export_report(rows, format="csv", compress=True) +``` + +Adding a fourth format is a one-line change to the `Literal`; the call sites read meaningfully (`format="parquet"` instead of `True, False, True`). + +**Correct option C (`Enum` when the modes carry behavior or constants):** + +```python +from enum import Enum + +class CompressionLevel(Enum): + NONE = 0 + FAST = 1 + BEST = 9 + +def export_report(rows: list[Row], *, level: CompressionLevel = CompressionLevel.NONE) -> bytes: + data = render_csv(rows) + if level is CompressionLevel.NONE: + return data + return gzip.compress(data, compresslevel=level.value) +``` + +The enum gives each variant a name *and* a meaningful value. Type checkers narrow on `is` comparisons. + +**`bool` parameters that are genuinely binary toggles are still okay** — but only when: + +- The flag is keyword-only (use `*` per `api-keyword-only-params`) +- The name clearly answers "what does True mean?" (`include_archived=True`, `strict=True`, `dry_run=True`) +- There's no plausible third mode coming +- The body doesn't fork into two near-duplicate paths + +```python +def list_users(*, include_archived: bool = False) -> list[User]: + if include_archived: + return query_all_users() + return query_active_users() +``` + +`include_archived=True` reads at the call site. The body is genuinely a small branch on a single SQL filter. + +**Heuristic:** read your call sites out loud. `export_report(rows, True, False)` fails the test. `export_report(rows, format="csv", compress=True)` passes. If you hear positional booleans, the API needs splitting or a `Literal`. diff --git a/skills/python-best-practices/rules/api-required-before-optional.md b/skills/python-best-practices/rules/api-required-before-optional.md index fd99701..85cf28c 100644 --- a/skills/python-best-practices/rules/api-required-before-optional.md +++ b/skills/python-best-practices/rules/api-required-before-optional.md @@ -3,6 +3,7 @@ title: Order Required Fields Before Optional Fields impact: HIGH impactDescription: Python enforces this at class-definition time tags: api, dataclasses, defaults +references: https://docs.python.org/3/library/dataclasses.html#dataclasses.dataclass, https://docs.python.org/3/library/dataclasses.html#dataclasses.KW_ONLY --- ## Order Required Fields Before Optional Fields diff --git a/skills/python-best-practices/rules/data-aware-datetimes.md b/skills/python-best-practices/rules/data-aware-datetimes.md new file mode 100644 index 0000000..ae27a13 --- /dev/null +++ b/skills/python-best-practices/rules/data-aware-datetimes.md @@ -0,0 +1,117 @@ +--- +title: Use Timezone-Aware Datetimes at Boundaries +impact: HIGH +impactDescription: prevents off-by-hours bugs across timezones, daylight saving, and storage +tags: data, datetime, timezone, boundaries +references: https://docs.python.org/3/library/datetime.html#aware-and-naive-objects, https://docs.python.org/3/library/zoneinfo.html, https://peps.python.org/pep-0615/ +--- + +## Use Timezone-Aware Datetimes at Boundaries + +A `datetime` with no `tzinfo` is **naive**: it has no opinion about which timezone it represents. Two naive datetimes that look identical may refer to different absolute moments. Naive datetimes leak into databases, JSON payloads, log lines, and inter-service messages and cause off-by-hours bugs that surface during DST transitions, on a different host, or when a user travels. + +The rule: at any boundary the value crosses (HTTP, DB, queue, file format, log line, comparison with another datetime), the datetime must be **timezone-aware**. Inside a tight piece of business logic, naive is acceptable only if every value in scope shares the same explicit assumption — and even then, attaching the timezone is usually clearer. + +**Default to UTC for storage and transport. Convert to local timezones only at display.** + +**Incorrect (`datetime.utcnow()` returns a naive datetime — silently loses the "UTC" claim):** + +```python +from datetime import datetime + +def stamp() -> datetime: + return datetime.utcnow() # naive! DeprecationWarning in 3.12+ +``` + +`datetime.utcnow()` is deprecated in Python 3.12 precisely because it returns a *naive* datetime that callers misuse as if it were UTC-aware. A serializer that interprets naive as local time will write the wrong value to the database. + +**Incorrect (`datetime.now()` is naive and host-local):** + +```python +from datetime import datetime + +start = datetime.now() # naive, in the host's local timezone +log.info("started", start=start) # serializes ambiguously +``` + +The same code on two hosts in different timezones records different timestamps for the same event. + +**Incorrect (mixing naive and aware in comparisons — `TypeError`):** + +```python +from datetime import datetime, timezone + +stored = datetime(2026, 4, 17, 12, 0) # naive +now = datetime.now(timezone.utc) # aware +if stored < now: # TypeError! + ... +``` + +The interpreter refuses to compare naive and aware datetimes — a guard against a class of bugs that would otherwise be silent. + +**Correct (UTC at every boundary):** + +```python +from datetime import datetime, timezone + +def stamp() -> datetime: + return datetime.now(timezone.utc) # aware, unambiguous + +start = datetime.now(timezone.utc) +log.info("started", start=start.isoformat()) # "2026-04-17T12:00:00+00:00" +``` + +`datetime.now(timezone.utc)` is the modern replacement for `datetime.utcnow()`. The result is aware and round-trips through `isoformat()` / `fromisoformat()` cleanly. + +**Correct (named local timezone via `zoneinfo` for display):** + +```python +from datetime import datetime, timezone +from zoneinfo import ZoneInfo + +stored = datetime.now(timezone.utc) # store in UTC +display = stored.astimezone(ZoneInfo("America/Los_Angeles")) # convert at display +print(display.strftime("%Y-%m-%d %H:%M %Z")) +``` + +`zoneinfo` (Python 3.9+, PEP 615) reads from the system tzdata; it handles DST and historical offsets correctly. Use named zones (`"America/Los_Angeles"`), not raw offsets (`-08:00`), so DST transitions resolve. + +**Correct (parsing user/API input — fail loudly on missing timezone):** + +```python +from datetime import datetime, timezone + +def parse_iso(s: str) -> datetime: + dt = datetime.fromisoformat(s) + if dt.tzinfo is None: + raise ValueError(f"datetime {s!r} is missing a timezone offset") + return dt.astimezone(timezone.utc) +``` + +If your callers can send naive datetimes, decide once whether to reject them or to assume a fixed zone — but never *silently* treat naive as UTC. + +**Pydantic / dataclasses:** + +```python +from datetime import datetime, timezone +from pydantic import BaseModel, AwareDatetime + +class Event(BaseModel): + occurred_at: AwareDatetime # Pydantic v2: rejects naive datetimes at validation +``` + +`pydantic.AwareDatetime` enforces the rule at the model boundary. The standard library doesn't ship a "must be aware" annotation; encode the constraint with a validator or rely on Pydantic. + +**Database guidance:** + +- PostgreSQL: use `TIMESTAMPTZ` (stores UTC). Driver returns aware datetimes. +- SQLite / MySQL: store ISO-8601 strings with `+00:00`, or store epoch milliseconds. +- ORMs: configure timezone-aware columns explicitly; defaults vary. + +**When naive is acceptable:** + +- Pure date arithmetic where time-of-day doesn't matter (`date`, not `datetime`) +- A small block of business logic where every value is naive and the timezone is documented in scope +- Integrating with a legacy system whose contract is naive — but convert at the boundary on the way out + +**Heuristic:** if the datetime is going to live longer than the function it's created in, it should be aware. Naive datetimes are a sharp local tool, never a transport format. diff --git a/skills/python-best-practices/rules/data-discriminated-unions.md b/skills/python-best-practices/rules/data-discriminated-unions.md index 27357c5..b928e5a 100644 --- a/skills/python-best-practices/rules/data-discriminated-unions.md +++ b/skills/python-best-practices/rules/data-discriminated-unions.md @@ -3,6 +3,7 @@ title: Use Discriminated Unions Over Optional Bags impact: CRITICAL impactDescription: makes impossible states unrepresentable tags: data, types, unions, modeling +references: https://docs.python.org/3/library/typing.html#typing.Literal, https://docs.pydantic.dev/latest/concepts/unions/#discriminated-unions --- ## Use Discriminated Unions Over Optional Bags @@ -55,6 +56,6 @@ PaymentState = PaymentIdle | PaymentProcessing | PaymentSettled Now `match payment.status:` narrows exactly, `transaction_id` is non-optional on the variants that have it, and impossible combinations (idle with a transaction ID, settled without a timestamp) are unrepresentable. -**With Pydantic:** use `Field(discriminator="status")` and a `status: Literal[...]` tag on each variant — Pydantic will validate and narrow automatically. +**With Pydantic** *(applicability: pydantic)*: use `Field(discriminator="status")` and a `status: Literal[...]` tag on each variant — Pydantic will validate and narrow automatically. **Null over sentinels:** don't invent `"none"` action values. `pending_action: PendingAction | None` beats `pending_action: Literal["none", "confirm-address", "select-shipping"]`. Absence is not an action. diff --git a/skills/python-best-practices/rules/data-encapsulate-mutable-state.md b/skills/python-best-practices/rules/data-encapsulate-mutable-state.md index cf3ca81..cd17f1f 100644 --- a/skills/python-best-practices/rules/data-encapsulate-mutable-state.md +++ b/skills/python-best-practices/rules/data-encapsulate-mutable-state.md @@ -1,15 +1,27 @@ --- -title: Encapsulate Mutable State in the Smallest Possible Scope +title: Encapsulate Mutable State in the Narrowest Clear Scope impact: HIGH impactDescription: limits the blast radius of state mutations -tags: data, state, encapsulation, closures +tags: data, state, encapsulation, scope +references: https://docs.python.org/3/reference/executionmodel.html#naming-and-binding --- -## Encapsulate Mutable State in the Smallest Possible Scope +## Encapsulate Mutable State in the Narrowest Clear Scope -If mutable state must exist, trap it where only the code that needs it can see it. A closure is better than an instance attribute; an instance attribute is better than a module-level global. Agents default to the loosest scope — push back. +If mutable state must exist, give it the narrowest scope where the code that needs it is still **clear**. The principle is "narrowest *clear* scope," not "always closures over instance attributes." A closure can be the right answer when the state is small, the interface is one or two callables, and there's nothing else to inspect or test. An instance attribute is the right answer when the state belongs to a domain object with identity, when multiple methods need to share it, or when you want it to be easy to inspect, type, serialize, or mock in tests. -**Incorrect (state visible to every method on the class):** +**Pick the smallest scope where the surrounding code still reads naturally:** + +| Scope | Use when | +|-------|----------| +| Local variable | State lives entirely inside one function call | +| Closure | A small handle of 1–2 callables; state must outlive a single call but doesn't need identity | +| Private instance attribute (`_name`) | State belongs to a domain object; multiple methods read/write it; you want introspection, typing, and serialization | +| Module-level global | Genuinely process-wide state — caches, registries (rare; prefer dependency injection) | + +Module-level globals deserve the most pushback. Closures and instance attributes are both legitimate; the choice depends on whether the state has identity worth naming. + +**Incorrect (state visible to every method on the class — too wide):** ```python from typing import Callable @@ -18,30 +30,22 @@ class DebouncedWriter: def __init__(self, callback: Callable[[], None], delay_ms: int = 300): self._callback = callback self._delay_ms = delay_ms - self._timeout_handle: TimerHandle | None = None # visible to all methods - - def queue_send(self, text: str) -> None: - # can touch _timeout_handle - ... - - def flush_now(self) -> None: - # can touch _timeout_handle - ... + self._timeout_handle: TimerHandle | None = None # touched by every method - def something_else(self) -> None: - # can also touch _timeout_handle — and nothing prevents a bug here - ... + def queue_send(self, text: str) -> None: ... + def flush_now(self) -> None: ... + def something_else(self) -> None: ... # nothing prevents a bug here ``` -Any method — including new ones added later — can read or mutate `_timeout_handle`. That's how invariants rot. +If only `queue_send` and `flush_now` need `_timeout_handle`, every other method is a potential source of a state bug. -**Correct (state trapped in a closure):** +**Correct option A (closure — state trapped behind a small handle):** ```python from dataclasses import dataclass from typing import Callable -@dataclass +@dataclass(frozen=True) class DebouncedAction: trigger: Callable[[], None] clear: Callable[[], None] @@ -53,12 +57,12 @@ def create_debounced_action(callback: Callable[[], None], delay_ms: int = 300) - nonlocal timeout if timeout is not None: timeout.cancel() - timeout = schedule_after(delay_ms, lambda: _fire(callback)) + timeout = schedule_after(delay_ms, _fire) - def _fire(cb: Callable[[], None]) -> None: + def _fire() -> None: nonlocal timeout timeout = None - cb() + callback() def clear() -> None: nonlocal timeout @@ -69,6 +73,48 @@ def create_debounced_action(callback: Callable[[], None], delay_ms: int = 300) - return DebouncedAction(trigger=trigger, clear=clear) ``` -Nothing outside the closure can reach `timeout`. The interface is two functions; the state is invisible. +Good fit when the only surface is `trigger` and `clear`, and nothing else needs to inspect `timeout`. + +**Correct option B (small focused class — when identity, inspection, or tests matter):** + +```python +from dataclasses import dataclass, field +from typing import Callable + +@dataclass +class DebouncedAction: + callback: Callable[[], None] + delay_ms: int = 300 + _timeout: TimerHandle | None = field(default=None, init=False, repr=False) + + def trigger(self) -> None: + if self._timeout is not None: + self._timeout.cancel() + self._timeout = schedule_after(self.delay_ms, self._fire) + + def _fire(self) -> None: + self._timeout = None + self.callback() + + def clear(self) -> None: + if self._timeout is not None: + self._timeout.cancel() + self._timeout = None +``` + +Good fit when: + +- Tests want to assert on `_timeout` being `None` +- A debugger should be able to print the object meaningfully +- Subclassing or replacing `_fire` matters +- The object will be serialized, logged, or compared + +Both versions are *narrower* than the original — neither lets unrelated methods touch the timer. The closure isn't categorically better; it's the right call when the surface is tiny and identity is irrelevant. + +**Heuristics for picking:** + +- One or two callables in the public interface, no introspection needed → closure +- Several methods sharing state, identity matters, tests want to peek → focused class with `_private` attributes +- State spans modules → reconsider the design before reaching for a module global -**When a class is the right tool:** when state belongs to a domain object with identity (a `User`, a `Session`), or when you need multiple methods to share state as a coherent unit. Then the state belongs on the instance — but still as `_private` attributes, not public ones. +**The wrong answer is a wide-open class.** Mutable state on a class that lets every method touch it is how invariants rot — regardless of whether the alternative is a closure or a smaller class. diff --git a/skills/python-best-practices/rules/data-mutable-defaults.md b/skills/python-best-practices/rules/data-mutable-defaults.md new file mode 100644 index 0000000..952cceb --- /dev/null +++ b/skills/python-best-practices/rules/data-mutable-defaults.md @@ -0,0 +1,90 @@ +--- +title: Never Use Mutable Default Arguments +impact: CRITICAL +impactDescription: prevents shared-state bugs across calls and instances +tags: data, defaults, mutability, dataclass, pydantic +references: https://docs.python.org/3/tutorial/controlflow.html#default-argument-values, https://docs.python.org/3/library/dataclasses.html#mutable-default-values, https://docs.pydantic.dev/latest/concepts/fields/#using-pydanticfield-to-describe-fields +--- + +## Never Use Mutable Default Arguments + +A default argument is evaluated **once**, when the `def`/class statement runs — not each call. A mutable default (`[]`, `{}`, `set()`, a dataclass instance) is therefore **shared across every call** that doesn't override it. The result is a footgun where appending to the "default" list on one call mutates the default for every subsequent call. The same trap exists for dataclass and Pydantic field defaults. + +Always use `None` (or a sentinel) and construct the mutable inside the body, or use `default_factory` for dataclasses / Pydantic fields. + +**Incorrect (function default — list shared across calls):** + +```python +def append_item(item: int, items: list[int] = []) -> list[int]: + items.append(item) + return items + +append_item(1) # [1] +append_item(2) # [1, 2] ← surprise: same list as before +append_item(3) # [1, 2, 3] +``` + +The `[]` was evaluated once at function-definition time. Every call without an explicit `items=` mutates the same object. + +**Correct (sentinel + per-call construction):** + +```python +def append_item(item: int, items: list[int] | None = None) -> list[int]: + if items is None: + items = [] + items.append(item) + return items + +append_item(1) # [1] +append_item(2) # [2] ← fresh list per call +``` + +**Incorrect (dataclass — bare mutable default raises `ValueError`, but tempting alternatives are bugs):** + +```python +from dataclasses import dataclass + +@dataclass +class User: + tags: list[str] = [] # ValueError: mutable default ... is not allowed: use default_factory +``` + +The dataclass decorator catches the obvious case. The dangerous variant is sneaking the same list past the check via a class attribute or a shared object — both of which produce the same shared-state bug at runtime. + +**Correct (dataclass — `default_factory`):** + +```python +from dataclasses import dataclass, field + +@dataclass +class User: + tags: list[str] = field(default_factory=list) + metadata: dict[str, str] = field(default_factory=dict) +``` + +`field(default_factory=list)` calls `list()` once per instance, giving each `User` its own list. + +**Incorrect (Pydantic — sharing a list across instances):** + +```python +from pydantic import BaseModel + +class Config(BaseModel): + tags: list[str] = [] # Pydantic deep-copies, but rely on intent, not accident +``` + +Pydantic v2 actually deep-copies the default for each instance, so this happens to work — but the intent is unclear, and the behavior depends on the Pydantic version. Make the factory explicit so future readers (and the type checker) see what you meant. + +**Correct (Pydantic — `Field(default_factory=...)`):** + +```python +from pydantic import BaseModel, Field + +class Config(BaseModel): + tags: list[str] = Field(default_factory=list) + settings: dict[str, str] = Field(default_factory=dict) +``` + +**Heuristic:** if the default value would compare `==` to itself across calls only because it's the *same object*, it's mutable — use `None` + body construction (functions) or `default_factory` (dataclasses, Pydantic). Tuples, frozensets, strings, ints, `None`, and `frozen=True` dataclasses are safe to use directly because they can't be mutated. + +**`from __future__ import annotations` does not help here.** The default-value evaluation rule is unrelated to annotation evaluation; the trap fires either way. diff --git a/skills/python-best-practices/rules/data-sentinel-when-none-is-valid.md b/skills/python-best-practices/rules/data-sentinel-when-none-is-valid.md new file mode 100644 index 0000000..808a59c --- /dev/null +++ b/skills/python-best-practices/rules/data-sentinel-when-none-is-valid.md @@ -0,0 +1,96 @@ +--- +title: Use a Sentinel Object When None Is a Real Domain Value +impact: MEDIUM-HIGH +impactDescription: distinguishes "no value passed" from "None passed deliberately" +tags: data, sentinel, none, optional, defaults +references: https://peps.python.org/pep-0661/, https://docs.python.org/3/library/typing.html#typing.Optional +--- + +## Use a Sentinel Object When `None` Is a Real Domain Value + +When `None` carries semantic meaning in your domain — "the user explicitly cleared this field," "no parent," "no assignee" — you can no longer use `None` as a "not provided" default. Reach for a private sentinel object instead. This complements `types-remove-redundant-optional`: that rule says drop `| None` when `None` is impossible; this rule says use a sentinel when `None` is meaningfully different from "not passed." + +PEP 661 documents the pattern (it didn't standardize a syntax, but the idiom is universal). The sentinel is a unique object you compare with `is`, never with `==`. + +**Incorrect (using `None` as both "absent" and "explicitly cleared"):** + +```python +def update_user(user_id: str, nickname: str | None = None) -> User: + user = db.get(user_id) + user.nickname = nickname # was the caller clearing the nickname, + db.save(user) # or did they just not pass it? + return user + +update_user("u1") # didn't touch nickname? cleared it? +update_user("u1", nickname=None) # same call — same ambiguity +update_user("u1", nickname="bob") # this one is clear +``` + +There is no way for the function to tell "the caller didn't mention nickname" from "the caller wants to clear it." That ambiguity has bitten every PATCH-style API ever written. + +**Correct (sentinel default + `None` meaning "clear"):** + +```python +from typing import Final + +class _Unset: + def __repr__(self) -> str: + return "" + +UNSET: Final = _Unset() + +def update_user( + user_id: str, + nickname: str | None | _Unset = UNSET, +) -> User: + user = db.get(user_id) + if nickname is not UNSET: + user.nickname = nickname # may be None (cleared) or a real string + db.save(user) + return user + +update_user("u1") # nickname untouched +update_user("u1", nickname=None) # nickname cleared +update_user("u1", nickname="bob") # nickname set to "bob" +``` + +Compare with `is`, not `==`, so callers can't accidentally pass an object that compares equal. + +**For Pydantic models — same pattern, same payoff.** Distinguishing "field omitted from PATCH payload" vs. "field set to null" is the canonical use case: + +```python +from typing import Any +from pydantic import BaseModel, Field + +class _Unset: + def __repr__(self) -> str: + return "" + +UNSET: Any = _Unset() # Any so it satisfies any field annotation + +class UserPatch(BaseModel): + nickname: str | None = Field(default=UNSET) + email: str = Field(default=UNSET) + + def changes(self) -> dict[str, object]: + return {k: v for k, v in self.model_dump().items() if v is not UNSET} +``` + +Now `UserPatch(nickname=None).changes() == {"nickname": None}` (clear) and `UserPatch().changes() == {}` (untouched). + +**`typing` exposes `Sentinel` (3.13+, PEP 661 follow-up).** When available, you can shorten the boilerplate: + +```python +# Python 3.13+ (proposed; check your interpreter) +from typing import Sentinel + +UNSET = Sentinel("UNSET") +``` + +Until that lands universally, the small `_Unset` class above is the portable form. + +**Don't use generic objects as sentinels.** `_UNSET = object()` works, but it gives no help to readers, type checkers, or debuggers. A small named class with a `__repr__` makes tracebacks readable. + +**Don't reach for sentinels when `None` is fine.** If `None` already means "absent" and there's no separate "explicitly cleared" state to distinguish, plain `nickname: str | None = None` is the right answer. The sentinel earns its complexity only when both meanings need to coexist. + +**Heuristic:** if your function or model needs to distinguish three states — "not provided," "provided as None," "provided as a real value" — you need a sentinel. Two states (`None` vs. value) is just `Optional`. diff --git a/skills/python-best-practices/rules/error-assert-debug-only.md b/skills/python-best-practices/rules/error-assert-debug-only.md new file mode 100644 index 0000000..cb3efb9 --- /dev/null +++ b/skills/python-best-practices/rules/error-assert-debug-only.md @@ -0,0 +1,74 @@ +--- +title: Use assert Only for Debug-Only Internal Invariants +impact: HIGH +impactDescription: prevents production checks from silently disappearing under -O +tags: error, assert, invariants, debug +references: https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement +--- + +## Use `assert` Only for Debug-Only Internal Invariants + +`assert` is a **debug-only** statement. The Python language reference is explicit: assertions emit no code when Python is run with `-O` (or `PYTHONOPTIMIZE`), so the check disappears in optimized builds. That makes `assert` the right tool for "this can never happen if my code is correct" — and the *wrong* tool for any check that must run in production. + +**Incorrect (using `assert` to enforce a runtime contract that must hold):** + +```python +def transfer_funds(account_id: str, amount: int) -> None: + assert amount > 0, "amount must be positive" # vanishes under -O + assert account_id, "account_id required" # vanishes under -O + ... +``` + +If this module is imported into a service deployed with `python -O`, both checks compile to nothing. A negative `amount` will sail through and corrupt state; the contract is gone. + +**Incorrect (using `assert` for input validation):** + +```python +def parse_request(payload: bytes) -> Request: + data = json.loads(payload) + assert "user_id" in data, "missing user_id" # never trust user input via assert + ... +``` + +User-supplied input must be validated with real exceptions — assertions can be optimized away, and even when present they raise `AssertionError`, which is a poor signal at a system boundary. + +**Correct (use `assert` only for "this is impossible if the rest of the code is correct"):** + +```python +def process_step(step: Step) -> Result: + # Step is a closed union; reaching the default branch is a programmer error. + if isinstance(step, InitStep): return init() + if isinstance(step, RunStep): return run() + if isinstance(step, DoneStep): return done() + assert False, f"unhandled Step variant: {step!r}" # debug aid only +``` + +The assertion documents the invariant. In development it fires loudly if the union grows a new variant; in production with `-O` it's gone, but at that point you're trusting the type system to have caught the gap. (For exhaustiveness specifically, `typing.assert_never` is sharper — see `error-assert-never-exhaustiveness`.) + +**Correct (use real exceptions for anything that must hold in production):** + +```python +def transfer_funds(account_id: str, amount: int) -> None: + if not account_id: + raise ValueError("account_id required") + if amount <= 0: + raise ValueError("amount must be positive") + ... +``` + +`ValueError` (or a domain-specific exception) is meaningful to callers, can be caught and handled, and survives `-O`. + +**Use `assert` when:** + +- The condition is a programmer-error invariant the type system can't fully express ("this list is sorted," "this counter is non-negative by construction") +- You want a sanity check during development that's free in production +- A `# noqa`-style "I know this can't happen" comment would otherwise be tempting + +**Use a real exception when:** + +- The check guards against caller mistakes (`ValueError`, `TypeError`) +- The input crosses a trust boundary (user input, external API, deserialized data) +- The failure mode is meaningful to the caller (`PermissionError`, `TimeoutError`, custom domain types) +- The check must run in production no matter how the interpreter is invoked + +If you can't articulate why losing the check under `-O` is acceptable, it shouldn't be an `assert`. diff --git a/skills/python-best-practices/rules/error-assert-invariants.md b/skills/python-best-practices/rules/error-assert-invariants.md deleted file mode 100644 index d84f29b..0000000 --- a/skills/python-best-practices/rules/error-assert-invariants.md +++ /dev/null @@ -1,63 +0,0 @@ ---- -title: Use assert for Invariants, Not RuntimeError -impact: MEDIUM-HIGH -impactDescription: documents assumptions and fails fast in development -tags: error, assert, invariants ---- - -## Use `assert` for Invariants, Not `RuntimeError('internal error')` - -`assert` documents "this can't happen" — and fails loudly in development if it does. `RuntimeError("internal error")` obscures the intent and fires the same in production, making programming errors look like runtime issues. Reserve exceptions for conditions the caller can reasonably respond to. - -**Incorrect (RuntimeError for impossible state):** - -```python -def process_step(step: Step) -> Result: - match step: - case InitStep(): return init() - case RunStep(): return run() - case DoneStep(): return done() - - raise RuntimeError("unexpected step") # shouldn't be reachable if types are right -``` - -If a new `Step` variant is added and this function isn't updated, `RuntimeError("unexpected step")` fires in production. It looks like a runtime problem — but it's a coding error the type system should have caught. - -**Correct (assert_never for exhaustiveness; assert for invariants):** - -```python -from typing import assert_never - -def process_step(step: Step) -> Result: - match step: - case InitStep(): return init() - case RunStep(): return run() - case DoneStep(): return done() - case _: assert_never(step) # type error at check time if new variant added -``` - -`assert_never(step)` is specifically designed for this — the checker will raise a type error if `Step` grows a new variant and the match isn't updated. - -**Use `assert` for preconditions the checker can't express:** - -```python -def binary_search(items: list[int], target: int) -> int: - assert items == sorted(items), "binary_search requires sorted input" - ... -``` - -This fails fast in development; in production with `-O`, asserts are stripped — which is appropriate because by then the invariant is trusted. - -**When to raise an exception instead:** - -- The caller could reasonably recover (`FileNotFoundError`, `ValidationError`) -- The input came from an untrusted boundary (user input, external API) -- The failure mode is meaningful to the caller (`PermissionError`, `TimeoutError`) - -**When to `assert`:** - -- "This can't happen if the rest of the code is correct" -- Internal invariants the checker can't fully enforce -- Sanity checks during development - -If the condition *can* happen, make it a real exception with a meaningful type. If it genuinely shouldn't happen, `assert` it. diff --git a/skills/python-best-practices/rules/error-assert-never-exhaustiveness.md b/skills/python-best-practices/rules/error-assert-never-exhaustiveness.md new file mode 100644 index 0000000..47057e2 --- /dev/null +++ b/skills/python-best-practices/rules/error-assert-never-exhaustiveness.md @@ -0,0 +1,84 @@ +--- +title: Use assert_never for Exhaustiveness Checks +impact: HIGH +impactDescription: turns "missing variant" into a type-check error +tags: error, exhaustiveness, typing, assert-never +references: https://docs.python.org/3/library/typing.html#typing.assert_never, https://peps.python.org/pep-0702/, https://typing.python.org/en/latest/spec/narrowing.html#assert-never-and-exhaustiveness-checking +--- + +## Use `assert_never` for Exhaustiveness Checks + +`typing.assert_never()` (Python 3.11+) is the right tool for "I've handled every variant of this union." Static checkers treat the call site as unreachable — if the union grows a new member, the checker reports the missed branch as a type error *before* the code ships. At runtime it raises `AssertionError`, so a missed case still fails loudly even if the checker is bypassed. + +This is **separate from** `assert` (the statement). `assert` is debug-only and can be stripped under `-O`; `assert_never` is a function call that always runs and is purpose-built for exhaustiveness narrowing. + +**Incorrect (RuntimeError for unreachable branch — checker doesn't help):** + +```python +from dataclasses import dataclass + +@dataclass +class InitStep: ... +@dataclass +class RunStep: ... +@dataclass +class DoneStep: ... + +Step = InitStep | RunStep | DoneStep + +def process_step(step: Step) -> Result: + if isinstance(step, InitStep): return init() + if isinstance(step, RunStep): return run() + if isinstance(step, DoneStep): return done() + raise RuntimeError(f"unexpected step: {step!r}") # checker can't tell this is exhaustive +``` + +If a future change adds `PausedStep` to the union, this function silently falls through to the `RuntimeError` at runtime. The type checker cannot see the gap because `RuntimeError` is not understood as an exhaustiveness assertion. + +**Incorrect (plain `assert False` — vanishes under `-O`):** + +```python +def process_step(step: Step) -> Result: + if isinstance(step, InitStep): return init() + if isinstance(step, RunStep): return run() + if isinstance(step, DoneStep): return done() + assert False, f"unhandled: {step!r}" # stripped under -O; checker doesn't narrow +``` + +Under `python -O`, the assertion compiles to nothing and the function falls off the end with `None` (a worse failure). Type checkers also do not treat plain `assert False` as a guaranteed-unreachable signal in the same way as `assert_never`. + +**Correct (`assert_never` — type error if the union grows, runtime error if reached):** + +```python +from typing import assert_never + +def process_step(step: Step) -> Result: + if isinstance(step, InitStep): return init() + if isinstance(step, RunStep): return run() + if isinstance(step, DoneStep): return done() + assert_never(step) # pyright/mypy: error if Step grows a new variant +``` + +If `Step` later becomes `InitStep | RunStep | DoneStep | PausedStep`, the checker reports that `step` is `PausedStep` at the `assert_never` call — the build breaks before the code ships. + +**Use with `match`/`case` the same way:** + +```python +from typing import assert_never + +def process_step(step: Step) -> Result: + match step: + case InitStep(): return init() + case RunStep(): return run() + case DoneStep(): return done() + case _: + assert_never(step) +``` + +**Where `assert_never` belongs:** + +- Closed sums: `Literal` unions, sealed dataclass hierarchies, discriminated unions +- Enum dispatch where every member must be handled +- Any place where "we covered every case" is a property the checker should enforce + +**Backport:** `typing.assert_never` is available from Python 3.11. On older versions, import from `typing_extensions` instead — the semantics are identical and both static checkers recognize either source. diff --git a/skills/python-best-practices/rules/error-context-managers.md b/skills/python-best-practices/rules/error-context-managers.md new file mode 100644 index 0000000..2b853af --- /dev/null +++ b/skills/python-best-practices/rules/error-context-managers.md @@ -0,0 +1,108 @@ +--- +title: Use with / async with for Resource Lifetimes +impact: HIGH +impactDescription: deterministic cleanup even on exceptions +tags: error, context-manager, resources, cleanup +references: https://docs.python.org/3/reference/compound_stmts.html#the-with-statement, https://docs.python.org/3/library/contextlib.html, https://peps.python.org/pep-0492/#asynchronous-context-managers-and-async-with +--- + +## Use `with` / `async with` for Resource Lifetimes + +Any object that owns a finite resource — file handles, network sockets, database connections, locks, temporary directories, HTTP clients, GPU contexts — should be acquired with `with` (or `async with`). The context-manager protocol guarantees `__exit__` runs even when the body raises, so cleanup happens deterministically. Manual `close()` calls forget to fire on exceptions, leak resources under failure, and are easy to misorder during refactors. + +The same applies to async resources: `async with` exists for `aiohttp` sessions, `httpx.AsyncClient`, `asyncio.Lock`, `anyio` task groups, and async DB drivers. Use it. + +**Incorrect (manual close — leaks on exception):** + +```python +def write_report(path: Path, rows: list[Row]) -> None: + f = path.open("w") + for row in rows: + f.write(format_row(row)) # if this raises, f is never closed + f.close() +``` + +If `format_row` raises midway, `f.close()` never runs. The handle leaks, the file may be left in a partially-written state, and on Windows the path is locked until garbage collection. + +**Incorrect (try/finally — works but verbose; the language gave you `with` for a reason):** + +```python +def write_report(path: Path, rows: list[Row]) -> None: + f = path.open("w") + try: + for row in rows: + f.write(format_row(row)) + finally: + f.close() +``` + +`with` collapses this to one line and removes the chance of forgetting `try`/`finally` next time. + +**Correct (`with` — close runs on success, exception, or early return):** + +```python +def write_report(path: Path, rows: list[Row]) -> None: + with path.open("w") as f: + for row in rows: + f.write(format_row(row)) +``` + +**Resources that must be acquired with a context manager:** + +- **Files:** `open(...)`, `tempfile.TemporaryDirectory()`, `tempfile.NamedTemporaryFile()` +- **Locks:** `threading.Lock()`, `threading.RLock()`, `asyncio.Lock()` +- **Network clients:** `httpx.Client()`, `httpx.AsyncClient()`, `aiohttp.ClientSession()` +- **Database connections / sessions:** `sqlite3.connect()`, SQLAlchemy `Session()`, async DB drivers +- **Subprocess pipes:** `subprocess.Popen` (3.2+ supports `with`) +- **Anything from `contextlib`:** `redirect_stdout`, `suppress`, `chdir` (3.11+), `closing()` + +**Async clients use `async with`:** + +```python +import httpx + +async def fetch_user(user_id: str) -> User: + async with httpx.AsyncClient() as client: + response = await client.get(f"/users/{user_id}") + return User.model_validate_json(response.content) +``` + +`async with` runs `__aexit__` even if `await client.get(...)` raises or the task is cancelled. + +**Stack multiple resources with `contextlib.ExitStack` (or one `with` statement):** + +```python +from contextlib import ExitStack + +def merge_files(inputs: list[Path], output: Path) -> None: + with ExitStack() as stack: + out = stack.enter_context(output.open("w")) + ins = [stack.enter_context(p.open()) for p in inputs] + for src in ins: + for line in src: + out.write(line) +``` + +`ExitStack` closes all resources in reverse order, even if one of the `enter_context` calls raises. + +**Write your own with `@contextmanager`:** + +```python +from contextlib import contextmanager +from collections.abc import Iterator + +@contextmanager +def acquire_lease(resource_id: str) -> Iterator[Lease]: + lease = lease_service.acquire(resource_id) + try: + yield lease + finally: + lease_service.release(lease.id) + +with acquire_lease("worker-42") as lease: + do_work(lease) +``` + +Use the async variant `@contextlib.asynccontextmanager` for resources awaited during acquisition or release. + +**Heuristic:** if you find yourself writing `try` / `finally` to call `close()`, `release()`, `disconnect()`, or `unlink()`, you almost certainly want `with` instead. diff --git a/skills/python-best-practices/rules/error-no-bare-except.md b/skills/python-best-practices/rules/error-no-bare-except.md new file mode 100644 index 0000000..9f3ba10 --- /dev/null +++ b/skills/python-best-practices/rules/error-no-bare-except.md @@ -0,0 +1,69 @@ +--- +title: Never Use Bare `except:` +impact: HIGH +impactDescription: bare except swallows KeyboardInterrupt, SystemExit, and async cancellation +tags: error, exceptions, bare-except +references: https://docs.python.org/3/tutorial/errors.html#handling-exceptions, https://docs.python.org/3/library/exceptions.html#BaseException, https://peps.python.org/pep-0008/#programming-recommendations +--- + +## Never Use Bare `except:` + +`except:` (with no exception type) catches **`BaseException`** — every exception in the interpreter, including the ones you must not silently swallow: + +- `KeyboardInterrupt` — Ctrl-C +- `SystemExit` — `sys.exit()`, normal interpreter shutdown +- `asyncio.CancelledError` (3.8+) and `BaseExceptionGroup` (3.11+) — async cancellation +- `MemoryError`, `GeneratorExit`, internal interpreter signals + +Bare `except` is broader than `except Exception:`, and the breadth is exactly the problem. PEP 8 calls it out: *"A bare `except:` clause will catch `SystemExit` and `KeyboardInterrupt` exceptions, making it harder to interrupt a program with Control-C."* `flake8` / `ruff` flag it as `E722`. Treat any bare `except:` as a bug. + +**Incorrect (bare except — Ctrl-C cannot interrupt this loop):** + +```python +while True: + try: + process_one() + except: # E722: bare except + log("retrying") + time.sleep(1) +``` + +A user who hits Ctrl-C is ignored. A `sys.exit()` from a child function is ignored. An async `CancelledError` is swallowed and the task hangs. + +**Incorrect (`except BaseException:` — same problem, spelled out):** + +```python +try: + do_work() +except BaseException: # don't catch BaseException directly either + log("done") +``` + +Catching `BaseException` is the explicit form of the same mistake. + +**Correct (catch what you actually intend to handle):** + +```python +while True: + try: + process_one() + except (TimeoutError, ConnectionError) as exc: + log("retrying", exc_info=exc) + time.sleep(1) + # KeyboardInterrupt, SystemExit, CancelledError propagate as they should +``` + +**Correct when you need a true catch-all (last line of defense — log and re-raise):** + +```python +def handle_request(req: Request) -> Response: + try: + return process(req) + except Exception: # NOT bare; excludes BaseException-only types + logger.exception("unhandled error in request handler") + raise # never swallow +``` + +Use `except Exception:` (not bare) at the outermost layer of a request handler, worker loop, or top-level entrypoint where you must log unexpected errors. Always re-raise — see `error-specific-exceptions` for the broader handler discussion, and `error-preserve-cancellation` for why `CancelledError` must reach the event loop. + +**The only legitimate use of `except BaseException:`** is in framework-level cleanup code that genuinely must run before the process exits (e.g., flushing logs in a process supervisor) — and even then, the handler must re-raise. If you're not writing that, you don't need it. diff --git a/skills/python-best-practices/rules/error-preserve-cancellation.md b/skills/python-best-practices/rules/error-preserve-cancellation.md index 90811ef..816f6d8 100644 --- a/skills/python-best-practices/rules/error-preserve-cancellation.md +++ b/skills/python-best-practices/rules/error-preserve-cancellation.md @@ -3,6 +3,7 @@ title: Preserve Asyncio Cancellation Semantics impact: HIGH impactDescription: avoids hung tasks and false-positive review flags tags: error, asyncio, cancellation, anyio +references: https://docs.python.org/3/library/asyncio-exceptions.html#asyncio.CancelledError, https://docs.python.org/3/library/exceptions.html#BaseException --- ## Preserve Asyncio Cancellation Semantics diff --git a/skills/python-best-practices/rules/error-specific-exceptions.md b/skills/python-best-practices/rules/error-specific-exceptions.md index 4286837..66065d1 100644 --- a/skills/python-best-practices/rules/error-specific-exceptions.md +++ b/skills/python-best-practices/rules/error-specific-exceptions.md @@ -3,11 +3,12 @@ title: Catch Specific Exception Types impact: HIGH impactDescription: prevents masking unrelated bugs tags: error, exceptions, defensive +references: https://docs.python.org/3/tutorial/errors.html#handling-exceptions, https://docs.python.org/3/library/exceptions.html#exception-hierarchy --- ## Catch Specific Exception Types -`except Exception:` catches everything — including the bugs you wanted to see. Agents default to broad handlers because "we should be resilient"; the cost is that `KeyError` from a typo in your own code gets silently swallowed alongside the network timeout you meant to handle. +Catch the specific exception types you actually intend to handle. A broad `except Exception:` catches every regular error in your codebase, including bugs you wanted to see. (For the even worse `except:` with no type at all — which also catches `KeyboardInterrupt` and `SystemExit` — see `error-no-bare-except`.) Agents default to broad handlers because "we should be resilient"; the cost is that `KeyError` from a typo in your own code gets silently swallowed alongside the network timeout you meant to handle. **Incorrect (bare except catches unrelated errors):** diff --git a/skills/python-best-practices/rules/error-trust-validated-state.md b/skills/python-best-practices/rules/error-trust-validated-state.md index 21ce52d..1d124bb 100644 --- a/skills/python-best-practices/rules/error-trust-validated-state.md +++ b/skills/python-best-practices/rules/error-trust-validated-state.md @@ -1,32 +1,60 @@ --- -title: Trust Validated State — Skip Redundant Defensive Checks +title: Trust Validated State Within the Same Trust Domain impact: MEDIUM -impactDescription: removes clutter and improves resilience -tags: error, validation, defensive +impactDescription: removes clutter without losing real safety +tags: error, validation, defensive, trust-boundary +references: https://docs.pydantic.dev/latest/concepts/validators/, https://docs.python.org/3/library/dataclasses.html#frozen-instances --- -## Trust Validated State — Skip Redundant Defensive Checks +## Trust Validated State Within the Same Trust Domain -Once a value has been validated at the boundary, internal code should trust it. Agents tend to add defensive checks "just in case" deep inside the call chain — the cost is noise, false branches, and the impression that validation elsewhere isn't reliable. +Once a value has been validated *and the validated object is immutable, locally constructed, and stays inside the same trust domain*, internal helpers can skip re-checking it. Outside that narrow case, defensive checks may still earn their keep — mutable objects can drift, plugin/untyped callers can construct bad instances, and rehydrated objects (from a cache, a queue, the database) cross a trust boundary even if the type name is the same. -**Incorrect (re-checking already-validated state):** +This rule is the cousin of `types-trust-the-checker`. The principle is the same — don't duplicate guarantees the system already provides — but state requires more care than types because state can change after validation. + +**Trust-domain checklist before deleting a defensive check:** + +1. **Immutability** — the object is frozen, or the field cannot be reassigned after construction. +2. **Locally constructed** — built by code you control, in this process, since the last validation. +3. **No untyped/plugin caller** — no place can produce the type without going through the validator. +4. **No rehydration since validation** — not loaded from cache, queue, RPC, or DB without re-validating. + +Meet all four → trust the invariant. Miss one → keep the check. + +**Incorrect (re-checking validated immutable state inside the same module):** ```python +from pydantic import BaseModel, model_validator + +class ValidatedOrder(BaseModel): + model_config = {"frozen": True} + items: list[Item] + total: int + + @model_validator(mode="after") + def _check(self) -> "ValidatedOrder": + if not self.items: + raise ValueError("order must have items") + if self.total < 0: + raise ValueError("total must be non-negative") + return self + + def fulfill_order(order: ValidatedOrder) -> None: - if order is None: # already enforced by type + if order is None: # type already excludes None raise ValueError("order required") - if not order.items: # already enforced by Pydantic validator + if not order.items: # validator guarantees this raise ValueError("order must have items") - if order.total < 0: # already enforced by validator + if order.total < 0: # validator guarantees this raise ValueError("total must be non-negative") for item in order.items: process(item) ``` -Every one of these checks was already done when `ValidatedOrder` was constructed. Repeating them says "I don't trust the validation." +Frozen + validated + local construction + no rehydration → the checks are noise. -**Correct (trust the invariants):** +**Correct (trust the invariant):** ```python def fulfill_order(order: ValidatedOrder) -> None: @@ -34,30 +62,58 @@ def fulfill_order(order: ValidatedOrder) -> None: process(item) ``` -Cleaner, faster, and if the validator changes, this function doesn't need updating. +**Keep defensive checks when any condition fails.** Concrete examples: + +**Mutable object:** + +```python +@dataclass # not frozen +class Cart: + items: list[Item] # callers can mutate after construction + +def checkout(cart: Cart) -> None: + if not cart.items: # KEEP — caller could have cleared the list + raise EmptyCartError() + ... +``` + +**Rehydrated from external storage:** + +```python +def replay_from_queue(payload: bytes) -> None: + order = ValidatedOrder.model_validate_json(payload) + # Validator just ran on this newly-constructed object → no extra check needed here. + ... + +def load_from_cache(key: str) -> ValidatedOrder: + raw = cache.get(key) + return ValidatedOrder.model_validate_json(raw) # KEEP validation — cache crossed a boundary +``` + +**Untyped or plugin caller:** -**When defensive checks are appropriate:** +```python +def run_user_plugin(plugin: Any) -> None: + config = plugin.get_config() + if not isinstance(config, ValidatedConfig): # KEEP — plugin might return anything + raise TypeError("plugin returned non-ValidatedConfig") + ... +``` -- At trust boundaries (first function to touch external data) -- Around code paths that can bypass the validator (direct construction in tests, deserialization shortcuts) -- When the invariant is load-bearing and a bug elsewhere could silently violate it (use `assert` to document) +**Document the invariant once when you do trust it.** A single `assert` (with the caveats from `error-assert-debug-only`) at the entry point can serve as documentation for readers without sprinkling defensive `if` chains through the body: ```python def fulfill_order(order: ValidatedOrder) -> None: - assert order.items, "ValidatedOrder must have items (validator guarantees this)" - # assertion documents the invariant; runs in dev, stripped in production + assert order.items, "ValidatedOrder validator guarantees non-empty items" for item in order.items: process(item) ``` -**Use defaults instead of assertions** when the goal is resilience rather than catching bugs: +**Resilience vs. strictness.** When the goal is "keep running on bad input" rather than "catch a bug," reach for a default rather than a check-and-raise: ```python -# defensive, resilient — use when the system should keep running +# resilient — fall back when config is missing or malformed timeout = config.timeout if config.timeout > 0 else DEFAULT_TIMEOUT - -# defensive, strict — use when a zero timeout means someone messed up -assert config.timeout > 0, "timeout must be positive" ``` -Pick based on whether you want the system to fail or to fall back. Don't do both. +Pick one — fail or fall back — not both. Defensive checks belong where the four-item checklist above doesn't pass. diff --git a/skills/python-best-practices/rules/error-validate-at-boundaries.md b/skills/python-best-practices/rules/error-validate-at-boundaries.md index 5334d7e..1ab06ec 100644 --- a/skills/python-best-practices/rules/error-validate-at-boundaries.md +++ b/skills/python-best-practices/rules/error-validate-at-boundaries.md @@ -3,6 +3,7 @@ title: Validate Input at System Boundaries impact: HIGH impactDescription: fails fast and prevents bad data from spreading tags: error, validation, boundaries +references: https://docs.pydantic.dev/latest/concepts/validators/ --- ## Validate Input at System Boundaries diff --git a/skills/python-best-practices/rules/imports-no-duplicates.md b/skills/python-best-practices/rules/imports-no-duplicates.md index 301577a..0971477 100644 --- a/skills/python-best-practices/rules/imports-no-duplicates.md +++ b/skills/python-best-practices/rules/imports-no-duplicates.md @@ -3,6 +3,7 @@ title: No Duplicate Imports impact: LOW impactDescription: prevents confusion and redundant work tags: imports, duplicates, cleanup +references: https://docs.astral.sh/ruff/rules/duplicate-bindings/ --- ## No Duplicate Imports diff --git a/skills/python-best-practices/rules/imports-no-side-effects.md b/skills/python-best-practices/rules/imports-no-side-effects.md new file mode 100644 index 0000000..e2ab381 --- /dev/null +++ b/skills/python-best-practices/rules/imports-no-side-effects.md @@ -0,0 +1,116 @@ +--- +title: Keep Modules Cheap to Import +impact: MEDIUM +impactDescription: faster CLIs, faster tests, faster worker startup +tags: imports, side-effects, startup, performance +references: https://docs.python.org/3/reference/import.html, https://docs.python.org/3/library/importlib.html +--- + +## Keep Modules Cheap to Import + +Importing a module should do as little as possible. Anything at module top-level — opening files, reading environment variables, building large data structures, connecting to databases, registering global handlers, hitting the network — runs every time *anything* in that module is imported. That cost compounds across CLIs (cold-start latency users feel), tests (collection time), worker pools (per-process startup), and serverless functions (cold-start time billed). Heavy import-time side effects also make modules hard to mock and hard to import in the wrong environment. + +Push side effects out of module scope into **functions, factories, lazy properties, or `__init__` methods** that callers invoke explicitly. + +**Incorrect (network call at import time):** + +```python +# config.py +import requests + +CONFIG = requests.get("https://config.example.com/v1").json() # runs on import +DB_URL = CONFIG["db_url"] +``` + +Importing `config` (or anything that imports it transitively) makes a network call. Tests that don't need config still pay. Offline CI breaks. Cold starts add a request worth of latency. + +**Incorrect (heavy initialization at import):** + +```python +# embeddings.py +import torch +from sentence_transformers import SentenceTransformer + +MODEL = SentenceTransformer("all-MiniLM-L6-v2") # 90 MB download + GPU init +``` + +Anyone who imports `embeddings` for a type, a constant, or a single helper triggers a 90 MB download and GPU init. The `--help` of a CLI takes seconds to render. + +**Incorrect (env-dependent failures at import):** + +```python +import os + +API_KEY = os.environ["MY_API_KEY"] # KeyError on import if unset +``` + +Now you cannot import this module to read its docstring without `MY_API_KEY` set. + +**Correct (lazy — pay only when the feature runs):** + +```python +# config.py +from functools import cache +import requests + +@cache +def get_config() -> dict[str, object]: + return requests.get("https://config.example.com/v1").json() + +def db_url() -> str: + return get_config()["db_url"] +``` + +`@cache` makes the first call do the work and subsequent calls hit the cache — same effective behavior as a module constant, but only when something actually asks for it. + +**Correct (lazy — model loaded on first use):** + +```python +# embeddings.py +from functools import cache + +@cache +def get_model() -> "SentenceTransformer": + from sentence_transformers import SentenceTransformer + return SentenceTransformer("all-MiniLM-L6-v2") + +def embed(text: str) -> list[float]: + return get_model().encode(text).tolist() +``` + +The heavy import lives inside the function (see `imports-top-of-file` for when inline imports are okay). The model loads on first `embed`, not on `import`. + +**Correct (env read at use, with a clear error):** + +```python +import os + +def api_key() -> str: + key = os.environ.get("MY_API_KEY") + if not key: + raise RuntimeError("MY_API_KEY is required to call this API") + return key +``` + +Now the module imports anywhere, and the missing env variable surfaces with a useful message at the call site. + +**What is fine to do at import time:** + +- Pure-Python constants: `MAX_RETRIES = 5`, `_NAME_RE = re.compile(r"...")` (compile is cheap and amortizes) +- Class and function definitions +- Cheap, deterministic, in-process work (registering a dataclass, creating a small lookup dict) +- Standard-library imports + +**What to push out of import time:** + +- Network or disk I/O +- Subprocess launches +- Loading large models, datasets, ML weights +- Reading environment variables that may be missing +- Connecting to databases or message queues +- Registering signal handlers, atexit hooks, observability sinks +- Heavy third-party imports the module doesn't use unconditionally + +**Test for it.** Run `python -c "import yourpackage"` with `--time` or in `cProfile`. If a single import takes more than ~100 ms or makes any network call, find what's running at module scope and defer it. + +**Heuristic:** if the work needs to happen *exactly once* per process, write a `@cache`-decorated function and call it on demand. If it needs to happen *every* call, write a regular function. Module-scope side effects are almost never the right choice — they're "every import" by accident, not "once per process" by design. diff --git a/skills/python-best-practices/rules/imports-remove-unused.md b/skills/python-best-practices/rules/imports-remove-unused.md index 9ef760e..d849949 100644 --- a/skills/python-best-practices/rules/imports-remove-unused.md +++ b/skills/python-best-practices/rules/imports-remove-unused.md @@ -3,6 +3,7 @@ title: Remove Unused Imports impact: LOW-MEDIUM impactDescription: prevents accidental dependencies and reduces noise tags: imports, dead-code, cleanup +references: https://docs.astral.sh/ruff/rules/unused-import/ --- ## Remove Unused Imports diff --git a/skills/python-best-practices/rules/imports-scope-helpers-to-usage.md b/skills/python-best-practices/rules/imports-scope-helpers-to-usage.md index 2ad760a..5857585 100644 --- a/skills/python-best-practices/rules/imports-scope-helpers-to-usage.md +++ b/skills/python-best-practices/rules/imports-scope-helpers-to-usage.md @@ -3,6 +3,7 @@ title: Scope Helpers and Constants to Their Usage Site impact: LOW-MEDIUM impactDescription: reduces namespace pollution and clarifies intent tags: structure, scope, helpers +references: https://peps.python.org/pep-0008/#imports --- ## Scope Helpers and Constants to Their Usage Site diff --git a/skills/python-best-practices/rules/imports-top-of-file.md b/skills/python-best-practices/rules/imports-top-of-file.md index b914696..4341b26 100644 --- a/skills/python-best-practices/rules/imports-top-of-file.md +++ b/skills/python-best-practices/rules/imports-top-of-file.md @@ -3,6 +3,7 @@ title: Place Imports at the Top of the File impact: LOW-MEDIUM impactDescription: makes dependencies visible at a glance tags: imports, structure, conventions +references: https://peps.python.org/pep-0008/#imports --- ## Place Imports at the Top of the File diff --git a/skills/python-best-practices/rules/perf-compile-regex-module-level.md b/skills/python-best-practices/rules/perf-compile-regex-module-level.md index b3bf174..3377c1d 100644 --- a/skills/python-best-practices/rules/perf-compile-regex-module-level.md +++ b/skills/python-best-practices/rules/perf-compile-regex-module-level.md @@ -3,6 +3,7 @@ title: Compile Static Regex Patterns at Module Level impact: MEDIUM impactDescription: avoids recompilation overhead on every call tags: perf, regex, module-level +references: https://docs.python.org/3/library/re.html#re.compile --- ## Compile Static Regex Patterns at Module Level diff --git a/skills/python-best-practices/rules/perf-generator-over-list.md b/skills/python-best-practices/rules/perf-generator-over-list.md index d7fe83e..d0dfc93 100644 --- a/skills/python-best-practices/rules/perf-generator-over-list.md +++ b/skills/python-best-practices/rules/perf-generator-over-list.md @@ -1,70 +1,86 @@ --- -title: Use Generators for Streaming Iteration +title: Stream with Generators When Memory or First-Result Latency Matters impact: MEDIUM -impactDescription: constant memory instead of O(n) -tags: perf, generators, memory +impactDescription: bounded memory and lazy evaluation for large or infinite sequences +tags: perf, generators, memory, streaming +references: https://docs.python.org/3/glossary.html#term-generator, https://docs.python.org/3/library/itertools.html --- -## Use Generators for Streaming Iteration +## Stream with Generators When Memory or First-Result Latency Matters -When you're iterating through values and only need them one at a time, a generator uses constant memory. Materializing to a list holds every intermediate value in memory — fine for 100 items, a problem for 100 million. +Generators trade materialization for laziness: they yield one value at a time, hold no intermediate list, and let the caller stop early. This is a **memory and streaming** rule, not a "generators are categorically better than lists" rule. When you need every result anyway, iterate the sequence more than once, want random access, or want to sort it — a list comprehension is the right tool, often clearer and sometimes faster (no `next()` overhead per element). -**Incorrect (materializes a full list just to iterate):** +**Reach for a generator when:** + +- The input is large enough that holding it twice in memory is a problem +- The input is unbounded (infinite stream, line-by-line file read) +- The consumer can stop early (`any()`, `next()`, `break`) +- The pipeline has multiple stages that would otherwise materialize between each + +**Reach for a list (or list comprehension) when:** + +- You need `len()` before iterating +- You iterate the same sequence more than once +- You need random access (`items[5]`) +- You'll sort the whole sequence anyway (sort materializes) +- The data is small and a list comprehension reads more clearly + +**Incorrect (materializes a multi-GB file for a count — OOMs at scale):** ```python -def process_log_lines(path: Path) -> int: - lines = path.read_text().splitlines() # loads entire file - parsed = [parse_line(line) for line in lines] # and a second full list - matching = [p for p in parsed if p.level == "ERROR"] # and a third +def count_errors(path: Path) -> int: + lines = path.read_text().splitlines() # full file in memory + parsed = [parse_line(line) for line in lines] # second full copy + matching = [p for p in parsed if p.level == "ERROR"] # third full copy return len(matching) ``` -Three full copies of the data in memory at once. For a 10GB log file, this OOMs. +For a 10GB log file, this OOMs. Three copies of the same data are alive at once. -**Correct (streaming):** +**Correct (streaming — constant memory regardless of file size):** ```python -def process_log_lines(path: Path) -> int: +def count_errors(path: Path) -> int: with path.open() as f: - count = 0 - for line in f: # iterator over lines - parsed = parse_line(line) - if parsed.level == "ERROR": - count += 1 - return count + return sum(1 for line in f if parse_line(line).level == "ERROR") ``` -One line at a time. Constant memory regardless of file size. +One line at a time. Constant memory. **Generator expressions for pipelines:** ```python with path.open() as f: - parsed = (parse_line(line) for line in f) # generator - errors = (p for p in parsed if p.level == "ERROR") # generator - count = sum(1 for _ in errors) # reduces without materializing + parsed = (parse_line(line) for line in f) + errors = (p for p in parsed if p.level == "ERROR") + count = sum(1 for _ in errors) ``` Each stage yields one value at a time; nothing is held in memory. -**When to materialize:** +**Lists are the right call when you'll re-iterate:** -- You need `len()` before iterating (generators don't have a length) -- You iterate the same sequence multiple times (generators exhaust) -- You need random access (`items[5]`) — generators are sequential only -- You need to sort the whole sequence (sort requires materialization anyway) +```python +# Generator would be wrong here — `users` is iterated twice. +users = [u for u in load_users() if u.active] +print(f"{len(users)} active users") +for user in users: + notify(user) +``` -**`yield` in functions for custom generators:** +A generator would exhaust on the first iteration and the second loop would run zero times — a real bug, not a performance issue. + +**Lists are also fine when the data is small:** ```python -def read_chunks(path: Path, size: int = 8192) -> Iterator[bytes]: - with path.open("rb") as f: - while chunk := f.read(size): - yield chunk +# 50 config entries, used once. A generator buys nothing here. +ports = [c.port for c in configs if c.enabled] ``` -`yield` builds a generator function — the caller iterates lazily. +Don't replace small list comprehensions with generators on stylistic grounds. + +**`itertools` for streaming pipelines:** -**`itertools` is your friend:** +`chain`, `islice`, `takewhile`, `dropwhile`, `tee`, `groupby` — all yield lazily. Reach for them when a pipeline is naturally streaming; skip them when the data already fits in memory and a comprehension is clearer. -`chain`, `islice`, `takewhile`, `dropwhile`, `tee`, `groupby` — all streaming. Use them instead of slicing/filtering materialized lists. +**Heuristic:** ask "what's the worst-case size of this sequence, and does the consumer touch each element exactly once?" If the answer is "large" and "yes," use a generator. Otherwise, write whichever reads more clearly — usually a list comprehension. diff --git a/skills/python-best-practices/rules/perf-isinstance-tuple-syntax.md b/skills/python-best-practices/rules/perf-isinstance-tuple-syntax.md index c6e8976..2006cb4 100644 --- a/skills/python-best-practices/rules/perf-isinstance-tuple-syntax.md +++ b/skills/python-best-practices/rules/perf-isinstance-tuple-syntax.md @@ -1,40 +1,45 @@ --- -title: Use Tuple Syntax in isinstance() Checks +title: Prefer Tuple Syntax in isinstance() Only on Profiled Hot Paths impact: LOW -impactDescription: tuple syntax is measurably faster than union syntax -tags: perf, isinstance, micro-optimization +impactDescription: tiny per-call savings; only relevant in tight loops +tags: perf, isinstance, micro-optimization, hot-path +references: https://docs.python.org/3/library/functions.html#isinstance, https://peps.python.org/pep-0604/ --- -## Use Tuple Syntax in `isinstance()` Checks +## Prefer Tuple Syntax in `isinstance()` Only on Profiled Hot Paths -`isinstance(x, (A, B, C))` and `isinstance(x, A | B | C)` both work. The tuple form is faster at runtime because the union form constructs a `types.UnionType` every call. For hot paths, prefer the tuple. +Both `isinstance(x, (A, B, C))` and `isinstance(x, A | B | C)` are correct and supported in Python 3.10+. They produce the same result. The tuple form is *marginally* faster on each call because the union form constructs a `types.UnionType` object, but the gap is small enough that it only matters inside loops you've actually profiled. **Do not blanket-rewrite a codebase from union to tuple syntax** — the noise is rarely worth the diff. -**Incorrect (union syntax has allocation overhead):** +This is a micro-optimization, not a correctness rule. Apply it only when: -```python -def is_primitive(x: object) -> bool: - return isinstance(x, int | float | str | bool) # builds a union type each call -``` +1. The check is inside a measured hot path (a tight loop, called millions of times per request, etc.) +2. You have profiling data showing `isinstance` is a meaningful share of the time +3. You'd otherwise reach for a more invasive change (rewriting the dispatch, caching results) + +In normal code, write whichever reads more naturally. `isinstance(x, int | float)` mirrors a type annotation and is a fine default. -**Correct (tuple has no per-call overhead):** +**Incorrect (rewriting `A | B` to `(A, B)` everywhere as a stylistic crusade):** ```python -def is_primitive(x: object) -> bool: - return isinstance(x, (int, float, str, bool)) +# A drive-by PR that flips every isinstance() in the codebase. +def is_numeric(x: object) -> bool: + return isinstance(x, (int, float)) # was: isinstance(x, int | float) ``` -Both forms are semantically equivalent. The tuple version is faster because Python constructs the union type on every call (in older versions) or does extra work comparing against it (in newer versions). +The diff is pure churn. Annotations elsewhere use `int | float`; the inconsistency makes the codebase harder to read and the savings are imperceptible outside hot paths. -**When the difference matters:** +**Correct (apply only on a measured hot path, with a named module-level tuple):** -- Called many times per second in a hot path -- Inside a tight inner loop +```python +# This validator runs once per row across ~10M rows in the ETL job — profiled. +_PRIMITIVE_TYPES = (int, float, str, bool) -**When it doesn't matter:** +def is_primitive(x: object) -> bool: + return isinstance(x, _PRIMITIVE_TYPES) +``` -- Called a few times per request -- Rare code paths +Caching the tuple at module scope and giving it a clear name documents the intent ("this check is hot"). Anywhere else, `isinstance(x, int | float)` is fine. -**Note on annotations vs. runtime checks:** the union syntax (`X | Y`) is idiomatic in type annotations and has zero cost there (annotations aren't evaluated at runtime with `from __future__ import annotations`). The tuple form is only better for the specific case of `isinstance()` calls — other places `X | Y` appears, prefer the union syntax. +**Do not rewrite for style alone.** A diff that flips `isinstance(x, A | B)` to `isinstance(x, (A, B))` across a codebase is pure churn — you lose the visual symmetry with type annotations and gain a few microseconds on a path that runs once. -**Apply consistently** — it's a simple swap, and codebases that use both forms interchangeably make profiling results less predictable. Pick the tuple form once for all `isinstance` checks and move on. +**Annotations are unaffected.** In type annotations, `X | Y` is the modern form (PEP 604). The tuple form is only relevant inside `isinstance()` / `issubclass()` calls — and only on hot paths. diff --git a/skills/python-best-practices/rules/perf-lru-cache-pure-fns.md b/skills/python-best-practices/rules/perf-lru-cache-pure-fns.md index 2ff5af2..fad835d 100644 --- a/skills/python-best-practices/rules/perf-lru-cache-pure-fns.md +++ b/skills/python-best-practices/rules/perf-lru-cache-pure-fns.md @@ -3,6 +3,7 @@ title: Use functools.lru_cache for Pure Functions impact: MEDIUM impactDescription: trades memory for CPU on repeatable computations tags: perf, lru-cache, caching, functools +references: https://docs.python.org/3/library/functools.html#functools.lru_cache, https://docs.python.org/3/library/functools.html#functools.cache --- ## Use `functools.lru_cache` for Pure Functions diff --git a/skills/python-best-practices/rules/perf-type-adapter-constant.md b/skills/python-best-practices/rules/perf-type-adapter-constant.md index c1f2988..97b3b69 100644 --- a/skills/python-best-practices/rules/perf-type-adapter-constant.md +++ b/skills/python-best-practices/rules/perf-type-adapter-constant.md @@ -2,11 +2,14 @@ title: Define TypeAdapter Instances at Module Level impact: MEDIUM impactDescription: avoids repeated schema construction -tags: perf, pydantic, type-adapter, module-level +tags: perf, pydantic, type-adapter, module-level, applicability:pydantic +references: https://docs.pydantic.dev/latest/api/type_adapter/ --- ## Define `TypeAdapter` Instances at Module Level +> **Applicability:** this rule is specific to Pydantic v2's `TypeAdapter`. The same principle applies to any object whose constructor does real work (`json.JSONDecoder` with custom hooks, `msgpack.Packer`, compiled templates) — the Pydantic example is the canonical case. + `pydantic.TypeAdapter` does real work on construction — it builds the validation schema for the target type. Inside a hot function, every call rebuilds it. Create it once at module scope and reuse. **Incorrect (rebuilt on every call):** diff --git a/skills/python-best-practices/rules/simplify-cached-property.md b/skills/python-best-practices/rules/simplify-cached-property.md index 8069b51..2b0ea70 100644 --- a/skills/python-best-practices/rules/simplify-cached-property.md +++ b/skills/python-best-practices/rules/simplify-cached-property.md @@ -1,13 +1,16 @@ --- -title: Use @cached_property for Expensive Derived Attributes +title: Use @cached_property Only When the Instance Supports It impact: MEDIUM -impactDescription: defers computation and avoids recomputation -tags: simplify, cached-property, performance +impactDescription: defers work safely; misuse causes races and silent staleness +tags: simplify, cached-property, performance, threading +references: https://docs.python.org/3/library/functools.html#functools.cached_property --- -## Use `@cached_property` for Expensive Derived Attributes +## Use `@cached_property` Only When the Instance Supports It -When an attribute is computed from other fields, is expensive, and doesn't change over the object's lifetime, `@cached_property` is the right tool. It defers computation until first access and caches the result — avoiding both wasted work when the attribute is never used and repeated work when it's used many times. +`@cached_property` is the right tool when the cached value is **derived from effectively immutable inputs**, the getter is **idempotent**, the class **has a writable `__dict__`**, and the instance is not shared across threads racing on first access. Outside that envelope, the convenience masks real bugs: stale caches when inputs mutate, `TypeError` on `__slots__` classes that omit `__dict__`, and duplicated computation when two threads hit the property simultaneously. + +The standard library docs are explicit about all of this. Read them once before adding the decorator. **Incorrect (plain method — recomputes on every call):** @@ -22,7 +25,7 @@ class Report: return compute_stats(self.rows) ``` -Every call re-walks `self.rows`. If the caller invokes `report.summary_stats()` ten times in a function, you pay ten times. +Every call re-walks `self.rows`. If a caller invokes `report.summary_stats()` ten times, you pay ten times. **Incorrect (eager computation in `__post_init__`):** @@ -37,37 +40,75 @@ class Report: You pay at construction time whether or not the caller ever reads `stats`. -**Correct (`@cached_property` — lazy and cached):** +**Incorrect (`@cached_property` on mutable inputs — silent staleness):** ```python from functools import cached_property -from dataclasses import dataclass +from dataclasses import dataclass, field @dataclass class Report: - rows: list[Row] + rows: list[Row] = field(default_factory=list) # mutable; callers can append @cached_property def summary_stats(self) -> Stats: return compute_stats(self.rows) + +r = Report() +r.summary_stats # caches based on empty rows +r.rows.append(new_row) # mutates input +r.summary_stats # still the old cached Stats — stale, no warning ``` -First access runs `compute_stats`; subsequent accesses return the cached result from `self.__dict__`. If no caller reads `summary_stats`, no work happens. +The cache lives in `r.__dict__["summary_stats"]`. Mutating `rows` does not invalidate it. + +**Incorrect (`@cached_property` on a `__slots__` class with no `__dict__`):** + +```python +from functools import cached_property + +class Point: + __slots__ = ("x", "y") # no __dict__ + + def __init__(self, x: int, y: int) -> None: + self.x = x + self.y = y + + @cached_property + def magnitude(self) -> float: + return (self.x ** 2 + self.y ** 2) ** 0.5 -**Caveats:** +Point(3, 4).magnitude +# TypeError: cannot use cached_property instance without the underlying attribute +# (no '__dict__' attribute on 'Point' to cache 'magnitude') +``` -- **Mutability:** if `self.rows` changes after the property is accessed, the cached value is stale. Use `@cached_property` only when the dependencies are effectively immutable. -- **Equality / hashing:** the cache lives in `__dict__`, so it persists across `copy()` unless you clear it. Include `compare=False` on the cache field if using dataclass comparisons. -- **`@property` is still right for cheap derivations** — accessor-like computations (`full_name`, `is_valid`) don't need caching and shouldn't use it. +`cached_property` writes the result into `instance.__dict__`. If the class doesn't have one, the call raises at first access. Either add `"__dict__"` to `__slots__` or use a different caching strategy (`@functools.lru_cache` on a top-level function, an explicit `_cache` field, etc.). -**Use `functools.lru_cache` for module-level pure functions:** +**Correct (lazy and cached, with the inputs effectively immutable):** ```python -from functools import lru_cache +from dataclasses import dataclass, field +from functools import cached_property -@lru_cache(maxsize=256) -def parse_version(s: str) -> Version: - ... +@dataclass(frozen=True) +class Report: + rows: tuple[Row, ...] # immutable container; cannot be mutated after construction + + @cached_property + def summary_stats(self) -> Stats: + return compute_stats(self.rows) ``` -`@cached_property` is the instance-method equivalent. +First access runs `compute_stats`; subsequent accesses return the cached result. Because `rows` is a frozen field of an immutable container, the cache cannot go stale. + +**Caveats to keep in mind:** + +- **Threading:** the docs warn that `cached_property` is not thread-safe. If two threads access the property for the first time at the same time, the getter may run twice. Use a lock, `functools.lru_cache` on a module-level function, or a one-shot `__post_init__` if the work must happen exactly once. +- **Mutability:** if any input the getter reads can change after first access, the cache is wrong. Make the inputs frozen, or stick with a plain method/`@property`. +- **Idempotency:** the getter must produce the same value for the same instance every time. No randomness, no time-dependence, no I/O whose result varies. +- **`__slots__`:** the class must keep `__dict__` available. Slot-only classes need `"__dict__"` in `__slots__`, or skip the decorator. +- **Equality / hashing:** the cached value lands in `__dict__`. Dataclass `eq=True` won't include it (only declared fields), but `copy.copy` carries the cache over — clear it manually if the copy's inputs differ. +- **`@property` is still right for cheap derivations** — accessor-like computations (`full_name`, `is_valid`) don't need caching. + +**For module-level pure functions, use `functools.lru_cache` / `functools.cache` instead** (see `perf-lru-cache-pure-fns`). `@cached_property` is the per-instance equivalent — and only a good fit when the instance meets every condition above. diff --git a/skills/python-best-practices/rules/simplify-remove-dead-code.md b/skills/python-best-practices/rules/simplify-remove-dead-code.md index 174e89e..c0ad02f 100644 --- a/skills/python-best-practices/rules/simplify-remove-dead-code.md +++ b/skills/python-best-practices/rules/simplify-remove-dead-code.md @@ -3,6 +3,7 @@ title: Remove Commented-Out and Dead Code impact: MEDIUM impactDescription: reduces confusion about intent tags: simplify, dead-code, cleanup +references: https://docs.astral.sh/ruff/rules/commented-out-code/ --- ## Remove Commented-Out and Dead Code diff --git a/skills/python-best-practices/rules/types-fix-errors-not-ignore.md b/skills/python-best-practices/rules/types-fix-errors-not-ignore.md index 0cdff13..ffd2b1e 100644 --- a/skills/python-best-practices/rules/types-fix-errors-not-ignore.md +++ b/skills/python-best-practices/rules/types-fix-errors-not-ignore.md @@ -3,6 +3,7 @@ title: Fix Type Errors, Don't Ignore Them impact: HIGH impactDescription: prevents masked errors from compounding tags: types, mypy, pyright, ignore +references: https://mypy.readthedocs.io/en/stable/error_codes.html, https://microsoft.github.io/pyright/#/comments --- ## Fix Type Errors, Don't Ignore Them diff --git a/skills/python-best-practices/rules/types-narrow-to-runtime-reality.md b/skills/python-best-practices/rules/types-narrow-to-runtime-reality.md index be2bc7e..b981f98 100644 --- a/skills/python-best-practices/rules/types-narrow-to-runtime-reality.md +++ b/skills/python-best-practices/rules/types-narrow-to-runtime-reality.md @@ -3,6 +3,7 @@ title: Narrow Type Signatures to Runtime Reality impact: MEDIUM impactDescription: eliminates unreachable branches and false permissiveness tags: types, narrowing, unions, design +references: https://docs.python.org/3/library/typing.html#typing.assert_never, https://typing.python.org/en/latest/spec/narrowing.html --- ## Narrow Type Signatures to Runtime Reality diff --git a/skills/python-best-practices/rules/types-remove-redundant-optional.md b/skills/python-best-practices/rules/types-remove-redundant-optional.md index 3931f26..09fecda 100644 --- a/skills/python-best-practices/rules/types-remove-redundant-optional.md +++ b/skills/python-best-practices/rules/types-remove-redundant-optional.md @@ -1,5 +1,5 @@ --- -title: Remove Redundant Optional Annotations +title: Remove Redundant `| None` When Values Are Guaranteed impact: MEDIUM impactDescription: eliminates false uncertainty in the type signature tags: types, optional, none, annotations diff --git a/skills/python-best-practices/rules/types-typeddict-over-dict-any.md b/skills/python-best-practices/rules/types-typeddict-over-dict-any.md index 597efb8..b62e707 100644 --- a/skills/python-best-practices/rules/types-typeddict-over-dict-any.md +++ b/skills/python-best-practices/rules/types-typeddict-over-dict-any.md @@ -3,6 +3,7 @@ title: Use TypedDict or Dataclass Instead of dict[str, Any] impact: CRITICAL impactDescription: restores type-checker coverage over config and payloads tags: types, typeddict, dataclass, any +references: https://docs.python.org/3/library/typing.html#typing.TypedDict, https://docs.pydantic.dev/latest/concepts/models/ --- ## Use TypedDict or Dataclass Instead of `dict[str, Any]` diff --git a/skills/python-best-practices/src/extract_tests.py b/skills/python-best-practices/src/extract_tests.py new file mode 100644 index 0000000..74c2aea --- /dev/null +++ b/skills/python-best-practices/src/extract_tests.py @@ -0,0 +1,168 @@ +#!/usr/bin/env python3 +"""Extract Incorrect/Correct example pairs from rule files into test-cases.json. + +The output is consumed by LLM eval pipelines that score whether an agent can +identify the failure mode shown in the Incorrect example, and produce code +matching the spirit of the Correct example. + +Schema (per entry): + { + "rule": "{filename without extension}", + "title": "{rule title}", + "impact": "{CRITICAL|HIGH|...}", + "tags": ["tag1", "tag2"], + "incorrect": "{first incorrect code block, language=python}", + "correct": "{first correct code block, language=python}", + "explanation": "{first paragraph of the rule body}" + } + +Rules without a clean Incorrect/Correct pair are skipped with a warning. + +Usage: + python src/extract_tests.py + +Run from the skill root. +""" + +from __future__ import annotations + +import json +import re +import sys +from dataclasses import dataclass +from pathlib import Path + +FRONTMATTER_RE = re.compile(r"^---\n(.*?)\n---\n(.*)$", re.DOTALL) +INCORRECT_BLOCK_RE = re.compile( + r"\*\*Incorrect[^*]*?\*\*[^\n]*\n+```python\n(.*?)```", + re.DOTALL, +) +CORRECT_BLOCK_RE = re.compile( + r"\*\*Correct[^*]*?\*\*[^\n]*\n+```python\n(.*?)```", + re.DOTALL, +) + + +@dataclass +class TestCase: + rule: str + title: str + impact: str + tags: list[str] + incorrect: str + correct: str + explanation: str + + +def parse_frontmatter(text: str) -> tuple[dict[str, str], str] | None: + match = FRONTMATTER_RE.match(text) + if match is None: + return None + fm_raw, body = match.group(1), match.group(2) + frontmatter: dict[str, str] = {} + for line in fm_raw.splitlines(): + if ":" not in line: + continue + key, _, value = line.partition(":") + frontmatter[key.strip()] = value.strip() + return frontmatter, body + + +def first_paragraph(body: str) -> str: + body = body.lstrip() + lines = body.split("\n", 1) + if lines and lines[0].startswith("## "): + body = lines[1] if len(lines) > 1 else "" + body = body.lstrip() + paragraph: list[str] = [] + for line in body.splitlines(): + stripped = line.strip() + if not stripped: + if paragraph: + break + continue + if stripped.startswith(("**", "```", "#", ">", "-")): + break + paragraph.append(stripped) + return " ".join(paragraph) + + +def extract(path: Path) -> TestCase | None: + text = path.read_text() + parsed = parse_frontmatter(text) + if parsed is None: + return None + frontmatter, body = parsed + + incorrect_match = INCORRECT_BLOCK_RE.search(body) + correct_match = CORRECT_BLOCK_RE.search(body) + if not incorrect_match or not correct_match: + return None + + title = frontmatter.get("title", path.stem) + impact = frontmatter.get("impact", "") + tags_raw = frontmatter.get("tags", "") + tags = [t.strip() for t in tags_raw.split(",") if t.strip()] + explanation = first_paragraph(body) + + return TestCase( + rule=path.stem, + title=title, + impact=impact, + tags=tags, + incorrect=incorrect_match.group(1).rstrip(), + correct=correct_match.group(1).rstrip(), + explanation=explanation, + ) + + +def main() -> int: + root = Path.cwd() + rules_dir = root / "rules" + output_path = root / "test-cases.json" + + if not rules_dir.exists(): + print(f"rules/ not found at {rules_dir}", file=sys.stderr) + return 1 + + cases: list[dict[str, object]] = [] + skipped: list[str] = [] + + for path in sorted(rules_dir.glob("*.md")): + if path.name.startswith("_"): + continue + case = extract(path) + if case is None: + skipped.append(path.name) + continue + cases.append( + { + "rule": case.rule, + "title": case.title, + "impact": case.impact, + "tags": case.tags, + "incorrect": case.incorrect, + "correct": case.correct, + "explanation": case.explanation, + } + ) + + payload = { + "generated_by": "src/extract_tests.py", + "count": len(cases), + "cases": cases, + } + output_path.write_text(json.dumps(payload, indent=2) + "\n") + + print( + f"wrote {output_path.name}: {len(cases)} cases extracted, " + f"{len(skipped)} skipped", + file=sys.stderr, + ) + for name in skipped: + print(f" skipped (no clean Incorrect/Correct pair): {name}", file=sys.stderr) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/skills/python-best-practices/src/validate.py b/skills/python-best-practices/src/validate.py new file mode 100644 index 0000000..c8c73d6 --- /dev/null +++ b/skills/python-best-practices/src/validate.py @@ -0,0 +1,200 @@ +#!/usr/bin/env python3 +"""Lint rule files for structural correctness. + +Checks: +- Frontmatter present, parseable, with required fields (`title`, `impact`, `tags`) +- `references` present when the rule body mentions a Python version, PEP, or + named third-party library (Pydantic, mypy, ruff, asyncio, etc.) that suggests + primary-source corroboration is needed +- `impact` is one of the documented levels +- Body starts with `## {title}` matching the frontmatter title +- Body contains an Incorrect/Correct example pair (heuristic: looks for the + bolded `**Incorrect` and `**Correct` markers) +- Filename prefix maps to a known section in `_sections.md` + +Exits with a non-zero status if any rule fails. + +Usage: + python src/validate.py + +Run from the skill root. +""" + +from __future__ import annotations + +import re +import sys +from dataclasses import dataclass, field +from pathlib import Path + +VALID_IMPACTS: frozenset[str] = frozenset( + {"CRITICAL", "HIGH", "MEDIUM-HIGH", "MEDIUM", "LOW-MEDIUM", "LOW"} +) +REQUIRED_FRONTMATTER: tuple[str, ...] = ("title", "impact", "tags") + +# Heuristics for "this rule depends on language/library behavior, so a +# primary-source reference is required". Any match → require `references`. +# +# Design intent: trigger on signals that are concretely version- or +# library-dependent (a specific stdlib API with known version semantics, a +# Pydantic v2 construct, a tool name) — *not* on bare module names like +# "typing" or "dataclasses" which appear in almost every rule body and would +# turn the validator into noise. +VERSION_OR_LIB_PATTERNS: tuple[re.Pattern[str], ...] = ( + # Concrete version mentions + re.compile(r"\bPython\s+3\.\d+\+?\b", re.IGNORECASE), + re.compile(r"\b3\.(?:9|10|11|12|13|14)\+\b"), + re.compile(r"\bPEP[-\s]?\d+\b", re.IGNORECASE), + # Version-sensitive stdlib APIs + re.compile(r"\bwarnings\.deprecated\b"), + re.compile(r"\bcached_property\b"), + re.compile(r"\bassert_never\b"), + re.compile(r"\bzoneinfo\b"), + re.compile(r"\bKW_ONLY\b"), + re.compile(r"\bExceptionGroup\b"), + re.compile(r"\btomllib\b"), + re.compile(r"\b@overload\b"), + # Asyncio cancellation semantics shifted across versions + re.compile(r"\bCancelledError\b"), + re.compile(r"\basyncio\b"), + # Third-party tools whose behavior the rule depends on + re.compile(r"\bpydantic\b", re.IGNORECASE), + re.compile(r"\bTypeAdapter\b"), + re.compile(r"\bBaseModel\b"), + re.compile(r"\bmodel_validator\b"), + re.compile(r"\bmodel_dump\b"), + re.compile(r"\bmypy\b"), + re.compile(r"\bpyright\b"), + re.compile(r"\bruff\b"), +) + +FRONTMATTER_RE = re.compile(r"^---\n(.*?)\n---\n(.*)$", re.DOTALL) +SECTION_RE = re.compile( + r"^## \d+\. .+? \((\w+)\)\s*\n", re.MULTILINE +) + + +@dataclass +class RuleIssue: + filename: str + issues: list[str] = field(default_factory=list) + + +def parse_frontmatter(text: str) -> tuple[dict[str, str], str] | None: + match = FRONTMATTER_RE.match(text) + if match is None: + return None + fm_raw, body = match.group(1), match.group(2) + frontmatter: dict[str, str] = {} + for line in fm_raw.splitlines(): + if ":" not in line: + continue + key, _, value = line.partition(":") + frontmatter[key.strip()] = value.strip() + return frontmatter, body + + +def load_section_prefixes(sections_path: Path) -> set[str]: + text = sections_path.read_text() + return {m.group(1).strip() for m in SECTION_RE.finditer(text)} + + +def needs_reference(body: str) -> str | None: + for pat in VERSION_OR_LIB_PATTERNS: + m = pat.search(body) + if m: + return m.group(0) + return None + + +def validate_rule( + path: Path, valid_prefixes: set[str] +) -> RuleIssue: + issue = RuleIssue(filename=path.name) + text = path.read_text() + + parsed = parse_frontmatter(text) + if parsed is None: + issue.issues.append("missing or malformed frontmatter") + return issue + frontmatter, body = parsed + + for required in REQUIRED_FRONTMATTER: + if required not in frontmatter or not frontmatter[required]: + issue.issues.append(f"missing required frontmatter field: {required!r}") + + impact = frontmatter.get("impact", "") + if impact and impact not in VALID_IMPACTS: + issue.issues.append( + f"impact {impact!r} not in {sorted(VALID_IMPACTS)}" + ) + + prefix = path.stem.split("-", 1)[0] + if prefix not in valid_prefixes: + issue.issues.append( + f"filename prefix {prefix!r} not in _sections.md prefixes " + f"({sorted(valid_prefixes)})" + ) + + title = frontmatter.get("title", "").strip() + body_stripped = body.lstrip() + first_line = body_stripped.split("\n", 1)[0] + # Compare titles ignoring backticks/whitespace — bodies usually format + # API names with code fences while frontmatter strings stay plain. + def _normalize(s: str) -> str: + return re.sub(r"\s+", " ", s.replace("`", "").strip()) + if title and not first_line.startswith("## "): + issue.issues.append( + f"body must start with '## {title}'; found {first_line!r}" + ) + elif title and _normalize(first_line[3:]) != _normalize(title): + issue.issues.append( + f"body heading {_normalize(first_line[3:])!r} " + f"does not match frontmatter title {_normalize(title)!r}" + ) + + if "**Incorrect" not in body: + issue.issues.append("body missing **Incorrect** example marker") + if "**Correct" not in body: + issue.issues.append("body missing **Correct** example marker") + + references = frontmatter.get("references", "").strip() + trigger = needs_reference(body) + if trigger and not references: + issue.issues.append( + f"body mentions {trigger!r} (version/library) but `references` is missing" + ) + + return issue + + +def main() -> int: + root = Path.cwd() + rules_dir = root / "rules" + sections_path = rules_dir / "_sections.md" + + if not sections_path.exists(): + print(f"_sections.md not found at {sections_path}", file=sys.stderr) + return 1 + + valid_prefixes = load_section_prefixes(sections_path) + failures = 0 + checked = 0 + + for path in sorted(rules_dir.glob("*.md")): + if path.name.startswith("_"): + continue + checked += 1 + result = validate_rule(path, valid_prefixes) + if result.issues: + failures += 1 + print(f"FAIL {result.filename}", file=sys.stderr) + for issue in result.issues: + print(f" - {issue}", file=sys.stderr) + + print(f"validated {checked} rules; {failures} failures", file=sys.stderr) + return 1 if failures else 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/skills/python-best-practices/test-cases.json b/skills/python-best-practices/test-cases.json new file mode 100644 index 0000000..d7ec121 --- /dev/null +++ b/skills/python-best-practices/test-cases.json @@ -0,0 +1,974 @@ +{ + "generated_by": "src/extract_tests.py", + "count": 71, + "cases": [ + { + "rule": "api-deprecated-aliases", + "title": "Keep Old Names as Deprecated Aliases", + "impact": "HIGH", + "tags": [ + "api", + "deprecation", + "compatibility" + ], + "incorrect": "# v1.0\ndef get_user(user_id: str) -> User: ...\n\n# v1.1\ndef fetch_user(user_id: str) -> User: ... # renamed \u2014 v1.0 callers now crash", + "correct": "import warnings\n\ndef fetch_user(user_id: str) -> User:\n ...\n\ndef get_user(user_id: str) -> User:\n warnings.warn(\n \"get_user is deprecated; use fetch_user instead.\",\n DeprecationWarning,\n stacklevel=2,\n )\n return fetch_user(user_id)", + "explanation": "Renaming a public function, class, or parameter is a breaking change. Users upgrade at their own pace; if the old name vanishes, they can't. Keep the old name as a deprecated alias for at least one release, pointing at the new name." + }, + { + "rule": "api-immutable-transforms", + "title": "Return New Collections from Transforms", + "impact": "HIGH", + "tags": [ + "api", + "immutability", + "mutation", + "transforms" + ], + "incorrect": "def filter_active(users: list[User]) -> list[User]:\n users[:] = [u for u in users if u.is_active] # mutates input!\n return users", + "correct": "def filter_active(users: list[User]) -> list[User]:\n return [u for u in users if u.is_active]", + "explanation": "A function called `filter_active(users)` that mutates `users` in place is a trap \u2014 the name says \"filter,\" the behavior says \"modify.\" Default to returning new collections. Reserve mutation for functions whose names make it unmistakable (`sort_in_place`, `update_items`)." + }, + { + "rule": "api-instance-vs-module-fn", + "title": "Choose the Simplest Namespace That Matches Ownership and Polymorphism", + "impact": "MEDIUM", + "tags": [ + "api", + "methods", + "functions", + "design", + "namespace" + ], + "incorrect": "def update_user_preferences(user: User, key: str, value: object) -> None:\n user.prefs[key] = value\n user.last_modified = now()\n\ndef get_user_display_name(user: User) -> str:\n return f\"{user.first_name} {user.last_name}\"", + "correct": "class User:\n def update_preference(self, key: str, value: object) -> None:\n self.prefs[key] = value\n self.last_modified = now()\n\n @property\n def display_name(self) -> str:\n return f\"{self.first_name} {self.last_name}\"", + "explanation": "Python lets the same logic live as a module-level function, an instance method, a `@classmethod`, a `@staticmethod`, a method on a `Protocol`, or a method on a `dataclass`. None of these is universally right. Pick the smallest namespace that captures **ownership** (does this operation belong to one object?) and **polymorphism** (will multiple types provide their own version?)." + }, + { + "rule": "api-keyword-only-params", + "title": "Use Keyword-Only Parameters for Optional Config", + "impact": "HIGH", + "tags": [ + "api", + "parameters", + "keyword-only", + "compatibility" + ], + "incorrect": "def fetch(url, timeout=30, retries=3, verify_ssl=True, backoff=1.5):\n ...\n\nfetch(\"https://api.example.com\", 60, 5, False)", + "correct": "def fetch(url, *, timeout=30, retries=3, verify_ssl=True, backoff=1.5):\n ...\n\nfetch(\"https://api.example.com\", timeout=60, retries=5, verify_ssl=False)", + "explanation": "Positional parameters lock in their order forever \u2014 adding a new parameter in the middle breaks every caller. Keyword-only parameters (after `*` in functions, after `_: KW_ONLY` in dataclasses) let you add, remove, or reorder without breaking callers. Agents default to positional; push back." + }, + { + "rule": "api-model-cohesion", + "title": "Keep Data Models Flat and Non-Redundant", + "impact": "MEDIUM", + "tags": [ + "api", + "models", + "cohesion" + ], + "incorrect": "from dataclasses import dataclass\n\n@dataclass\nclass ToolReturn:\n tool_name: str # also in parent\n call_id: str # also in parent\n content: dict[str, object] # single-key wrapper around return_value\n return_value: dict[str, object] # duplicated in content\n messages: list[Message] # always contains exactly one Message\n\n@dataclass\nclass ToolCall:\n tool_name: str\n call_id: str\n return_part: ToolReturn", + "correct": "from dataclasses import dataclass\n\n@dataclass\nclass ToolReturn:\n content: object # the actual return value, unwrapped\n message: Message\n\n@dataclass\nclass ToolCall:\n tool_name: str\n call_id: str\n return_part: ToolReturn", + "explanation": "Data models drift when fields duplicate each other, wrap single values in unnecessary containers, or mirror fields from the parent structure. Each duplicate is a second source of truth that can go stale. Each single-key wrapper adds access ceremony for no gain." + }, + { + "rule": "api-no-boolean-flag-params", + "title": "Avoid Boolean Flag Parameters in Public APIs", + "impact": "HIGH", + "tags": [ + "api", + "parameters", + "booleans", + "literal", + "enum" + ], + "incorrect": "def export_report(rows: list[Row], to_csv: bool = True, compress: bool = False) -> bytes:\n if to_csv:\n data = render_csv(rows)\n else:\n data = render_json(rows)\n if compress:\n data = gzip.compress(data)\n return data\n\nexport_report(rows, True, False) # what does True/False mean here?\nexport_report(rows, False, True) # JSON, compressed? CSV, compressed? Reader can't tell.", + "correct": "def export_csv(rows: list[Row]) -> bytes: ...\ndef export_json(rows: list[Row]) -> bytes: ...\n\ndef with_compression(data: bytes) -> bytes:\n return gzip.compress(data)\n\n# call site\ndata = with_compression(export_csv(rows))", + "explanation": "A boolean parameter is a binary mode switch hiding behind a generic type. The call site `download(url, True, False, True)` is unreadable, the function body branches on the flag with two near-duplicate code paths, and adding a third mode later requires breaking the API. This is the function-level cousin of `data-explicit-variants`: when behavior meaningfully changes on a flag, prefer split functions or a `Literal`/`Enum` parameter." + }, + { + "rule": "api-no-private-access", + "title": "Don't Access Private Attributes", + "impact": "HIGH", + "tags": [ + "api", + "privacy", + "coupling" + ], + "incorrect": "from some_lib import Client\n\nclient = Client()\n# peeking at a private attribute because there's no public way\nretry_count = client._retry_state[\"count\"]\nclient._pool.clear() # mutating private state", + "correct": "from some_lib import Client\n\nclient = Client()\nretry_count = client.stats.retries # public property\nclient.reset_pool() # public method", + "explanation": "`_prefixed` names are the author's contract: \"this is internal, it may change.\" Reaching into another module's or class's private attributes couples your code to implementation details you weren't invited into. Use the public API, or ask the owner to expose what you need." + }, + { + "rule": "api-required-before-optional", + "title": "Order Required Fields Before Optional Fields", + "impact": "HIGH", + "tags": [ + "api", + "dataclasses", + "defaults" + ], + "incorrect": "from dataclasses import dataclass\n\n@dataclass\nclass Tool:\n name: str\n description: str = \"\"\n version: str # TypeError: non-default argument follows default argument", + "correct": "from dataclasses import dataclass\n\n@dataclass\nclass Tool:\n name: str\n version: str\n description: str = \"\"", + "explanation": "Python's dataclass implementation requires fields without defaults to precede fields with defaults \u2014 trying to put an optional field before a required one is a `TypeError` at class-definition time. More importantly, the order communicates intent: required first, defaults last." + }, + { + "rule": "api-underscore-for-private", + "title": "Underscore Prefix for Private Names", + "impact": "HIGH", + "tags": [ + "api", + "privacy", + "public-api" + ], + "incorrect": "# mymodule.py\ndef format_date(d):\n return _to_iso_string(d)\n\ndef to_iso_string(d): # helper \u2014 but no underscore, so it's public\n return d.isoformat()\n\n__all__ = [\"format_date\", \"to_iso_string\"] # accidentally exported", + "correct": "# mymodule.py\ndef format_date(d):\n return _to_iso_string(d)\n\ndef _to_iso_string(d):\n return d.isoformat()\n\n__all__ = [\"format_date\"]", + "explanation": "Names that start with `_` are internal. Names that don't are public \u2014 and public means \"backward-compatible forever unless deprecated.\" Agents tend to leave implementation details public because there's no language-level enforcement; underscore them on the way in, not after they've leaked." + }, + { + "rule": "data-aware-datetimes", + "title": "Use Timezone-Aware Datetimes at Boundaries", + "impact": "HIGH", + "tags": [ + "data", + "datetime", + "timezone", + "boundaries" + ], + "incorrect": "from datetime import datetime\n\ndef stamp() -> datetime:\n return datetime.utcnow() # naive! DeprecationWarning in 3.12+", + "correct": "from datetime import datetime, timezone\n\ndef stamp() -> datetime:\n return datetime.now(timezone.utc) # aware, unambiguous\n\nstart = datetime.now(timezone.utc)\nlog.info(\"started\", start=start.isoformat()) # \"2026-04-17T12:00:00+00:00\"", + "explanation": "A `datetime` with no `tzinfo` is **naive**: it has no opinion about which timezone it represents. Two naive datetimes that look identical may refer to different absolute moments. Naive datetimes leak into databases, JSON payloads, log lines, and inter-service messages and cause off-by-hours bugs that surface during DST transitions, on a different host, or when a user travels." + }, + { + "rule": "data-delete-dead-variants", + "title": "Delete Dead Variants", + "impact": "MEDIUM", + "tags": [ + "data", + "types", + "unions", + "dead-code" + ], + "incorrect": "from typing import Literal\n\nOrderStatus = Literal[\"open\", \"paid\", \"shipped\", \"archived\"]\n\ndef render_status(status: OrderStatus) -> str:\n match status:\n case \"open\": return \"Awaiting payment\"\n case \"paid\": return \"Preparing to ship\"\n case \"shipped\": return \"In transit\"\n case \"archived\": return \"Archived\" # when does this branch ever run?", + "correct": "from typing import Literal\n\nOrderStatus = Literal[\"open\", \"paid\", \"shipped\"]\n\ndef render_status(status: OrderStatus) -> str:\n match status:\n case \"open\": return \"Awaiting payment\"\n case \"paid\": return \"Preparing to ship\"\n case \"shipped\": return \"In transit\"", + "explanation": "If a type has a variant that is never constructed \u2014 a `status: Literal[\"open\", \"closed\", \"archived\"]` where `\"archived\"` is never set \u2014 delete the variant. Agents leave them behind \"in case we need them later.\" The result is defensive branches in every consumer for a state that cannot occur." + }, + { + "rule": "data-derive-dont-store", + "title": "Derive, Don't Store", + "impact": "CRITICAL", + "tags": [ + "data", + "state", + "derivation", + "architecture" + ], + "incorrect": "from dataclasses import dataclass\n\n@dataclass\nclass ThreadState:\n was_interrupted: bool\n did_assistant_finish: bool\n did_assistant_error: bool\n was_tool_call_only: bool\n\ndef should_show_footer(state: ThreadState) -> bool:\n return (\n state.did_assistant_finish\n and not state.was_interrupted\n and not state.did_assistant_error\n and not state.was_tool_call_only\n )", + "correct": "def should_show_footer(events: list[SessionEvent]) -> bool:\n latest = get_latest_assistant_message(events)\n if latest is None:\n return False\n return (\n latest.completed\n and not latest.error\n and latest.finish_reason != \"tool_calls\"\n )", + "explanation": "Every boolean you add doubles the theoretical state space. When a value can be computed from data you already have, do not store it. Agents are tempted to cache derived values \"for performance\" \u2014 the cost is multiple mutation sites that must stay in sync, and they won't." + }, + { + "rule": "data-discriminated-unions", + "title": "Use Discriminated Unions Over Optional Bags", + "impact": "CRITICAL", + "tags": [ + "data", + "types", + "unions", + "modeling" + ], + "incorrect": "from dataclasses import dataclass\nfrom typing import Literal\n\n@dataclass\nclass PaymentState:\n status: Literal[\"idle\", \"processing\", \"settled\"]\n gateway: Literal[\"stripe\", \"paypal\"] | None = None\n transaction_id: str | None = None\n initiated_at: str | None = None\n settled_at: str | None = None", + "correct": "from dataclasses import dataclass\nfrom typing import Literal\n\n@dataclass\nclass PaymentIdle:\n status: Literal[\"idle\"] = \"idle\"\n\n@dataclass\nclass PaymentProcessing:\n gateway: Literal[\"stripe\", \"paypal\"]\n transaction_id: str\n initiated_at: str\n status: Literal[\"processing\"] = \"processing\"\n\n@dataclass\nclass PaymentSettled:\n gateway: Literal[\"stripe\", \"paypal\"]\n transaction_id: str\n settled_at: str\n status: Literal[\"settled\"] = \"settled\"\n\nPaymentState = PaymentIdle | PaymentProcessing | PaymentSettled", + "explanation": "Every optional field is a question the rest of the codebase must answer every time it touches the data. Agents tend to add optional fields as features grow, creating models where half the combinations are semantically invalid. Use a tagged (discriminated) union so the type system enforces which fields travel together." + }, + { + "rule": "data-encapsulate-mutable-state", + "title": "Encapsulate Mutable State in the Narrowest Clear Scope", + "impact": "HIGH", + "tags": [ + "data", + "state", + "encapsulation", + "scope" + ], + "incorrect": "from typing import Callable\n\nclass DebouncedWriter:\n def __init__(self, callback: Callable[[], None], delay_ms: int = 300):\n self._callback = callback\n self._delay_ms = delay_ms\n self._timeout_handle: TimerHandle | None = None # touched by every method\n\n def queue_send(self, text: str) -> None: ...\n def flush_now(self) -> None: ...\n def something_else(self) -> None: ... # nothing prevents a bug here", + "correct": "from dataclasses import dataclass\nfrom typing import Callable\n\n@dataclass(frozen=True)\nclass DebouncedAction:\n trigger: Callable[[], None]\n clear: Callable[[], None]\n\ndef create_debounced_action(callback: Callable[[], None], delay_ms: int = 300) -> DebouncedAction:\n timeout: TimerHandle | None = None\n\n def trigger() -> None:\n nonlocal timeout\n if timeout is not None:\n timeout.cancel()\n timeout = schedule_after(delay_ms, _fire)\n\n def _fire() -> None:\n nonlocal timeout\n timeout = None\n callback()\n\n def clear() -> None:\n nonlocal timeout\n if timeout is not None:\n timeout.cancel()\n timeout = None\n\n return DebouncedAction(trigger=trigger, clear=clear)", + "explanation": "If mutable state must exist, give it the narrowest scope where the code that needs it is still **clear**. The principle is \"narrowest *clear* scope,\" not \"always closures over instance attributes.\" A closure can be the right answer when the state is small, the interface is one or two callables, and there's nothing else to inspect or test. An instance attribute is the right answer when the state belongs to a domain object with identity, when multiple methods need to share it, or when you want it to be easy to inspect, type, serialize, or mock in tests." + }, + { + "rule": "data-explicit-variants", + "title": "Create Explicit Variants Instead of Mode Flags", + "impact": "CRITICAL", + "tags": [ + "data", + "api", + "architecture", + "variants" + ], + "incorrect": "from dataclasses import dataclass\nfrom typing import Literal\n\n@dataclass\nclass MessageComposer:\n on_submit: Callable[[str], None]\n mode: Literal[\"channel\", \"thread\", \"dm_thread\", \"edit\", \"forward\"]\n channel_id: str | None = None\n dm_id: str | None = None\n message_id: str | None = None\n\n def render(self) -> Frame:\n if self.mode == \"dm_thread\":\n extra = AlsoSendToDMField(self.dm_id)\n elif self.mode == \"thread\":\n extra = AlsoSendToChannelField(self.channel_id)\n else:\n extra = None\n if self.mode == \"edit\":\n actions = EditActions()\n elif self.mode == \"forward\":\n actions = ForwardActions()\n else:\n actions = DefaultActions()\n return Frame(extra, actions)", + "correct": "from dataclasses import dataclass\n\n@dataclass\nclass ChannelComposer:\n channel_id: str\n on_submit: Callable[[str], None]\n\n def render(self) -> Frame:\n return Frame(extra=None, actions=DefaultActions())\n\n@dataclass\nclass ThreadComposer:\n channel_id: str\n on_submit: Callable[[str], None]\n\n def render(self) -> Frame:\n return Frame(\n extra=AlsoSendToChannelField(self.channel_id),\n actions=DefaultActions(),\n )\n\n@dataclass\nclass EditMessageComposer:\n message_id: str\n on_submit: Callable[[str], None]\n\n def render(self) -> Frame:\n return Frame(extra=None, actions=EditActions())", + "explanation": "When a class starts growing `is_thread`, `is_editing`, `is_forwarding` flags \u2014 or a mode parameter like `mode: Literal[\"thread\", \"edit\", \"forward\"]` \u2014 stop. Each flag doubles the possible states; each mode check adds conditional logic at every call site. Split into explicit subclasses or sibling classes instead." + }, + { + "rule": "data-mutable-defaults", + "title": "Never Use Mutable Default Arguments", + "impact": "CRITICAL", + "tags": [ + "data", + "defaults", + "mutability", + "dataclass", + "pydantic" + ], + "incorrect": "def append_item(item: int, items: list[int] = []) -> list[int]:\n items.append(item)\n return items\n\nappend_item(1) # [1]\nappend_item(2) # [1, 2] \u2190 surprise: same list as before\nappend_item(3) # [1, 2, 3]", + "correct": "def append_item(item: int, items: list[int] | None = None) -> list[int]:\n if items is None:\n items = []\n items.append(item)\n return items\n\nappend_item(1) # [1]\nappend_item(2) # [2] \u2190 fresh list per call", + "explanation": "A default argument is evaluated **once**, when the `def`/class statement runs \u2014 not each call. A mutable default (`[]`, `{}`, `set()`, a dataclass instance) is therefore **shared across every call** that doesn't override it. The result is a footgun where appending to the \"default\" list on one call mutates the default for every subsequent call. The same trap exists for dataclass and Pydantic field defaults." + }, + { + "rule": "data-mutation-contract", + "title": "Pick a Mutation Contract", + "impact": "HIGH", + "tags": [ + "data", + "mutation", + "functions", + "contracts" + ], + "incorrect": "def with_pending_action(state: AppState, action: str) -> AppState:\n state.pending_action = action # mutation\n return state # and return", + "correct": "def apply_pending_action(state: AppState, action: str) -> None:\n state.pending_action = action", + "explanation": "A function that mutates its input *and* returns the same reference gives callers no way to tell whether to use the return value or the original. Pick one: mutate and return `None`, or clone and return the new value. Never both." + }, + { + "rule": "data-newtype-for-ids", + "title": "Brand Primitive IDs With NewType", + "impact": "MEDIUM", + "tags": [ + "data", + "types", + "newtype", + "domain" + ], + "incorrect": "UserId = str\nTeamId = str\n\ndef fetch_user(user_id: UserId) -> User: ...\n\nteam_id: TeamId = \"team_xyz\"\nfetch_user(team_id) # type checker is fine with this \u2014 runtime crash", + "correct": "from typing import NewType\n\nUserId = NewType(\"UserId\", str)\nTeamId = NewType(\"TeamId\", str)\n\ndef fetch_user(user_id: UserId) -> User: ...\n\nteam_id = TeamId(\"team_xyz\")\nfetch_user(team_id) # type error: TeamId is not UserId", + "explanation": "When `user_id` and `team_id` are both `str`, a function accepting `UserId` will happily take a `TeamId` and fail at runtime \u2014 or worse, silently return wrong data. `NewType` makes them distinct at the type level without runtime overhead." + }, + { + "rule": "data-phased-composition", + "title": "Phase Related Optional Fields Into Nested Structs", + "impact": "HIGH", + "tags": [ + "data", + "modeling", + "optional", + "dataclasses" + ], + "incorrect": "from dataclasses import dataclass\n\n@dataclass\nclass UserProfile:\n first_name: str | None = None\n last_name: str | None = None\n email: str | None = None\n phone: str | None = None\n company: str | None = None\n job_title: str | None = None\n billing_address: str | None = None\n billing_city: str | None = None\n billing_zip: str | None = None\n card_last4: str | None = None\n card_brand: str | None = None\n card_expires: str | None = None", + "correct": "from dataclasses import dataclass\n\n@dataclass\nclass Identity:\n first_name: str\n last_name: str\n email: str\n phone: str | None = None\n\n@dataclass\nclass Employment:\n company: str\n job_title: str\n\n@dataclass\nclass Billing:\n address: str\n city: str\n zip_code: str\n card_last4: str\n card_brand: str\n card_expires: str\n\n@dataclass\nclass UserProfile:\n identity: Identity | None = None\n employment: Employment | None = None\n billing: Billing | None = None", + "explanation": "When fields are \"all present or all absent\" in practice, don't model them as eight independent optionals at the top level. Agents tend to flatten everything into one class with `firstName: str | None`, `lastName: str | None`, etc. \u2014 which means every consumer writes `profile.first_name or defaults.first_name` eight times, and the type says nothing about which fields co-occur." + }, + { + "rule": "data-sentinel-when-none-is-valid", + "title": "Use a Sentinel Object When None Is a Real Domain Value", + "impact": "MEDIUM-HIGH", + "tags": [ + "data", + "sentinel", + "none", + "optional", + "defaults" + ], + "incorrect": "def update_user(user_id: str, nickname: str | None = None) -> User:\n user = db.get(user_id)\n user.nickname = nickname # was the caller clearing the nickname,\n db.save(user) # or did they just not pass it?\n return user\n\nupdate_user(\"u1\") # didn't touch nickname? cleared it?\nupdate_user(\"u1\", nickname=None) # same call \u2014 same ambiguity\nupdate_user(\"u1\", nickname=\"bob\") # this one is clear", + "correct": "from typing import Final\n\nclass _Unset:\n def __repr__(self) -> str:\n return \"\"\n\nUNSET: Final = _Unset()\n\ndef update_user(\n user_id: str,\n nickname: str | None | _Unset = UNSET,\n) -> User:\n user = db.get(user_id)\n if nickname is not UNSET:\n user.nickname = nickname # may be None (cleared) or a real string\n db.save(user)\n return user\n\nupdate_user(\"u1\") # nickname untouched\nupdate_user(\"u1\", nickname=None) # nickname cleared\nupdate_user(\"u1\", nickname=\"bob\") # nickname set to \"bob\"", + "explanation": "When `None` carries semantic meaning in your domain \u2014 \"the user explicitly cleared this field,\" \"no parent,\" \"no assignee\" \u2014 you can no longer use `None` as a \"not provided\" default. Reach for a private sentinel object instead. This complements `types-remove-redundant-optional`: that rule says drop `| None` when `None` is impossible; this rule says use a sentinel when `None` is meaningfully different from \"not passed.\"" + }, + { + "rule": "error-assert-debug-only", + "title": "Use assert Only for Debug-Only Internal Invariants", + "impact": "HIGH", + "tags": [ + "error", + "assert", + "invariants", + "debug" + ], + "incorrect": "def transfer_funds(account_id: str, amount: int) -> None:\n assert amount > 0, \"amount must be positive\" # vanishes under -O\n assert account_id, \"account_id required\" # vanishes under -O\n ...", + "correct": "def process_step(step: Step) -> Result:\n # Step is a closed union; reaching the default branch is a programmer error.\n if isinstance(step, InitStep): return init()\n if isinstance(step, RunStep): return run()\n if isinstance(step, DoneStep): return done()\n assert False, f\"unhandled Step variant: {step!r}\" # debug aid only", + "explanation": "`assert` is a **debug-only** statement. The Python language reference is explicit: assertions emit no code when Python is run with `-O` (or `PYTHONOPTIMIZE`), so the check disappears in optimized builds. That makes `assert` the right tool for \"this can never happen if my code is correct\" \u2014 and the *wrong* tool for any check that must run in production." + }, + { + "rule": "error-assert-never-exhaustiveness", + "title": "Use assert_never for Exhaustiveness Checks", + "impact": "HIGH", + "tags": [ + "error", + "exhaustiveness", + "typing", + "assert-never" + ], + "incorrect": "from dataclasses import dataclass\n\n@dataclass\nclass InitStep: ...\n@dataclass\nclass RunStep: ...\n@dataclass\nclass DoneStep: ...\n\nStep = InitStep | RunStep | DoneStep\n\ndef process_step(step: Step) -> Result:\n if isinstance(step, InitStep): return init()\n if isinstance(step, RunStep): return run()\n if isinstance(step, DoneStep): return done()\n raise RuntimeError(f\"unexpected step: {step!r}\") # checker can't tell this is exhaustive", + "correct": "from typing import assert_never\n\ndef process_step(step: Step) -> Result:\n if isinstance(step, InitStep): return init()\n if isinstance(step, RunStep): return run()\n if isinstance(step, DoneStep): return done()\n assert_never(step) # pyright/mypy: error if Step grows a new variant", + "explanation": "`typing.assert_never()` (Python 3.11+) is the right tool for \"I've handled every variant of this union.\" Static checkers treat the call site as unreachable \u2014 if the union grows a new member, the checker reports the missed branch as a type error *before* the code ships. At runtime it raises `AssertionError`, so a missed case still fails loudly even if the checker is bypassed." + }, + { + "rule": "error-consolidate-try-except", + "title": "Consolidate try/except Blocks with the Same Handler", + "impact": "MEDIUM-HIGH", + "tags": [ + "error", + "exceptions", + "duplication" + ], + "incorrect": "def load_config(path: Path) -> Config | None:\n try:\n raw = path.read_text()\n except FileNotFoundError:\n logger.warning(\"config missing: %s\", path)\n return None\n\n try:\n data = json.loads(raw)\n except json.JSONDecodeError:\n logger.warning(\"config invalid json: %s\", path)\n return None\n\n try:\n return Config(**data)\n except ValidationError:\n logger.warning(\"config validation failed: %s\", path)\n return None", + "correct": "def load_config(path: Path) -> Config | None:\n try:\n raw = path.read_text()\n data = json.loads(raw)\n return Config(**data)\n except (FileNotFoundError, json.JSONDecodeError, ValidationError) as e:\n logger.warning(\"config load failed: %s (%s)\", path, e)\n return None", + "explanation": "When multiple adjacent operations raise the same exception and need the same handling, merge them into one block. Separate blocks duplicate the handler \u2014 and if the handling logic ever changes, you now need to update N places." + }, + { + "rule": "error-context-managers", + "title": "Use with / async with for Resource Lifetimes", + "impact": "HIGH", + "tags": [ + "error", + "context-manager", + "resources", + "cleanup" + ], + "incorrect": "def write_report(path: Path, rows: list[Row]) -> None:\n f = path.open(\"w\")\n for row in rows:\n f.write(format_row(row)) # if this raises, f is never closed\n f.close()", + "correct": "def write_report(path: Path, rows: list[Row]) -> None:\n with path.open(\"w\") as f:\n for row in rows:\n f.write(format_row(row))", + "explanation": "Any object that owns a finite resource \u2014 file handles, network sockets, database connections, locks, temporary directories, HTTP clients, GPU contexts \u2014 should be acquired with `with` (or `async with`). The context-manager protocol guarantees `__exit__` runs even when the body raises, so cleanup happens deterministically. Manual `close()` calls forget to fire on exceptions, leak resources under failure, and are easy to misorder during refactors." + }, + { + "rule": "error-inherit-base-exceptions", + "title": "Inherit New Exceptions from Existing Base Exceptions", + "impact": "MEDIUM-HIGH", + "tags": [ + "error", + "exceptions", + "inheritance", + "compatibility" + ], + "incorrect": "class ToolError(Exception): ...\nclass ToolTimeoutError(ToolError): ...\nclass ToolValidationError(ToolError): ...\n\n# New failure mode added in v2:\nclass ToolRateLimitError(Exception): # doesn't inherit from ToolError\n ...\n\n# Existing caller:\ntry:\n run_tool(t, args)\nexcept ToolError: # no longer catches ToolRateLimitError\n retry()", + "correct": "class ToolError(Exception): ...\nclass ToolTimeoutError(ToolError): ...\nclass ToolValidationError(ToolError): ...\nclass ToolRateLimitError(ToolError): # fits the hierarchy\n ...\n\n# Existing caller still works:\ntry:\n run_tool(t, args)\nexcept ToolError: # catches ToolRateLimitError too\n retry()", + "explanation": "When adding a new exception type to a module that already has an exception hierarchy, inherit from the relevant base. Callers that catch the base will continue to catch the new type; skipping the base forces every caller to add a new `except` branch." + }, + { + "rule": "error-no-bare-except", + "title": "Never Use Bare `except:`", + "impact": "HIGH", + "tags": [ + "error", + "exceptions", + "bare-except" + ], + "incorrect": "while True:\n try:\n process_one()\n except: # E722: bare except\n log(\"retrying\")\n time.sleep(1)", + "correct": "while True:\n try:\n process_one()\n except (TimeoutError, ConnectionError) as exc:\n log(\"retrying\", exc_info=exc)\n time.sleep(1)\n # KeyboardInterrupt, SystemExit, CancelledError propagate as they should", + "explanation": "`except:` (with no exception type) catches **`BaseException`** \u2014 every exception in the interpreter, including the ones you must not silently swallow:" + }, + { + "rule": "error-preserve-cancellation", + "title": "Preserve Asyncio Cancellation Semantics", + "impact": "HIGH", + "tags": [ + "error", + "asyncio", + "cancellation", + "anyio" + ], + "incorrect": "async def fetch_with_retry() -> Result | None:\n try:\n return await upstream.get()\n except BaseException: # catches CancelledError\n logger.warning(\"fetch failed\")\n return None # task now \"completes\" despite being cancelled", + "correct": "async def fetch_with_retry() -> Result | None:\n try:\n return await upstream.get()\n except asyncio.CancelledError:\n raise # cancellation must propagate\n except Exception:\n logger.warning(\"fetch failed\", exc_info=True)\n return None", + "explanation": "Cancellation in asyncio is delivered by raising `CancelledError` inside the running task. Swallow it and the task hangs past its lifetime; false-flag code that already handles it correctly and you waste review cycles and churn working code." + }, + { + "rule": "error-raise-from-for-chains", + "title": "Use raise ... from to Preserve Exception Causality", + "impact": "MEDIUM", + "tags": [ + "error", + "exceptions", + "traceback", + "chaining" + ], + "incorrect": "def load_config(path: Path) -> Config:\n try:\n raw = path.read_text()\n except FileNotFoundError:\n raise ConfigError(f\"config missing: {path}\") # loses the original FileNotFoundError context", + "correct": "def load_config(path: Path) -> Config:\n try:\n raw = path.read_text()\n except FileNotFoundError as e:\n raise ConfigError(f\"config missing: {path}\") from e", + "explanation": "When you catch one exception and raise another, include `from original` to preserve the chain. Without it, the traceback prints \"During handling of the above exception, another exception occurred\" \u2014 which is usually right, but the explicit form is clearer and survives `__cause__` suppression in some runtimes." + }, + { + "rule": "error-repr-in-messages", + "title": "Use !r Format for Identifiers in Error Messages", + "impact": "MEDIUM", + "tags": [ + "error", + "formatting", + "messages", + "repr" + ], + "incorrect": "raise ValueError(f\"Tool {tool_name} not found in registry\")\n# \"Tool not found in registry\" \u2014 did tool_name have leading/trailing spaces? was it empty?\n# \"Tool None not found in registry\" \u2014 was the literal string \"None\" or actual None?", + "correct": "raise ValueError(f\"Tool {tool_name!r} not found in registry\")\n# \"Tool '' not found in registry\" \u2014 clearly empty string\n# \"Tool 'my tool' not found in registry\" \u2014 spaces visible\n# \"Tool None not found in registry\" \u2014 unambiguously the None sentinel", + "explanation": "`{name!r}` calls `repr(name)` \u2014 producing `'foo'` instead of `foo`, `42` instead of `42`, `None` instead of nothing. Use it for identifiers (names, paths, IDs) in error messages so values are clearly delimited and edge cases (empty strings, whitespace-only names, `None`) render visibly." + }, + { + "rule": "error-specific-exceptions", + "title": "Catch Specific Exception Types", + "impact": "HIGH", + "tags": [ + "error", + "exceptions", + "defensive" + ], + "incorrect": "def fetch_user(user_id: str) -> User | None:\n try:\n response = http.get(f\"/users/{user_id}\")\n return parse_user(response.json())\n except Exception: # catches everything \u2014 including your own bugs\n return None", + "correct": "def fetch_user(user_id: str) -> User | None:\n try:\n response = http.get(f\"/users/{user_id}\")\n except (HTTPError, TimeoutError):\n return None\n return parse_user(response.json()) # bugs here propagate as they should", + "explanation": "Catch the specific exception types you actually intend to handle. A broad `except Exception:` catches every regular error in your codebase, including bugs you wanted to see. (For the even worse `except:` with no type at all \u2014 which also catches `KeyboardInterrupt` and `SystemExit` \u2014 see `error-no-bare-except`.) Agents default to broad handlers because \"we should be resilient\"; the cost is that `KeyError` from a typo in your own code gets silently swallowed alongside the network timeout you meant to handle." + }, + { + "rule": "error-trust-validated-state", + "title": "Trust Validated State Within the Same Trust Domain", + "impact": "MEDIUM", + "tags": [ + "error", + "validation", + "defensive", + "trust-boundary" + ], + "incorrect": "from pydantic import BaseModel, model_validator\n\nclass ValidatedOrder(BaseModel):\n model_config = {\"frozen\": True}\n items: list[Item]\n total: int\n\n @model_validator(mode=\"after\")\n def _check(self) -> \"ValidatedOrder\":\n if not self.items:\n raise ValueError(\"order must have items\")\n if self.total < 0:\n raise ValueError(\"total must be non-negative\")\n return self\n\n\ndef fulfill_order(order: ValidatedOrder) -> None:\n if order is None: # type already excludes None\n raise ValueError(\"order required\")\n if not order.items: # validator guarantees this\n raise ValueError(\"order must have items\")\n if order.total < 0: # validator guarantees this\n raise ValueError(\"total must be non-negative\")\n\n for item in order.items:\n process(item)", + "correct": "def fulfill_order(order: ValidatedOrder) -> None:\n for item in order.items:\n process(item)", + "explanation": "Once a value has been validated *and the validated object is immutable, locally constructed, and stays inside the same trust domain*, internal helpers can skip re-checking it. Outside that narrow case, defensive checks may still earn their keep \u2014 mutable objects can drift, plugin/untyped callers can construct bad instances, and rehydrated objects (from a cache, a queue, the database) cross a trust boundary even if the type name is the same." + }, + { + "rule": "error-validate-at-boundaries", + "title": "Validate Input at System Boundaries", + "impact": "HIGH", + "tags": [ + "error", + "validation", + "boundaries" + ], + "incorrect": "def process_order(order_id: str) -> None:\n if not order_id:\n raise ValueError(\"order_id required\")\n order = load_order(order_id)\n fulfill(order)\n\ndef load_order(order_id: str) -> Order:\n if not order_id: # checked again\n raise ValueError(\"order_id required\")\n ...\n\ndef fulfill(order: Order) -> None:\n if not order.id: # and again\n raise ValueError(\"order has no id\")\n ...", + "correct": "# boundary: the API handler\ndef handle_fulfill_request(req: Request) -> Response:\n try:\n body = FulfillRequest.model_validate(req.json()) # Pydantic does the work\n except ValidationError as e:\n return error_response(400, str(e))\n\n process_order(body.order_id)\n return success_response()\n\n# internal: takes a validated value, trusts it\ndef process_order(order_id: OrderId) -> None:\n order = load_order(order_id)\n fulfill(order)", + "explanation": "Validate once, at the edge \u2014 not repeatedly in every internal function. Agents tend to sprinkle defensive checks throughout the call chain \"in case something got through.\" Push validation to the boundary (API handler, CLI entrypoint, deserialization), then trust the validated value." + }, + { + "rule": "imports-no-duplicates", + "title": "No Duplicate Imports", + "impact": "LOW", + "tags": [ + "imports", + "duplicates", + "cleanup" + ], + "incorrect": "import json\nfrom typing import Any\nfrom pathlib import Path\n\n# ... later in the file, after a later edit ...\nfrom pathlib import Path # duplicate\nfrom pathlib import Path as P # different alias, same underlying import", + "correct": "import json\nfrom typing import Any\nfrom pathlib import Path", + "explanation": "Two imports of the same name are either redundant (if they're identical) or a sign that a refactor left both in place. Either way, delete one. Tools flag this, but agents sometimes add a new import on top of an existing one without checking." + }, + { + "rule": "imports-no-side-effects", + "title": "Keep Modules Cheap to Import", + "impact": "MEDIUM", + "tags": [ + "imports", + "side-effects", + "startup", + "performance" + ], + "incorrect": "# config.py\nimport requests\n\nCONFIG = requests.get(\"https://config.example.com/v1\").json() # runs on import\nDB_URL = CONFIG[\"db_url\"]", + "correct": "# config.py\nfrom functools import cache\nimport requests\n\n@cache\ndef get_config() -> dict[str, object]:\n return requests.get(\"https://config.example.com/v1\").json()\n\ndef db_url() -> str:\n return get_config()[\"db_url\"]", + "explanation": "Importing a module should do as little as possible. Anything at module top-level \u2014 opening files, reading environment variables, building large data structures, connecting to databases, registering global handlers, hitting the network \u2014 runs every time *anything* in that module is imported. That cost compounds across CLIs (cold-start latency users feel), tests (collection time), worker pools (per-process startup), and serverless functions (cold-start time billed). Heavy import-time side effects also make modules hard to mock and hard to import in the wrong environment." + }, + { + "rule": "imports-optional-dependencies", + "title": "Handle Optional Dependencies Explicitly", + "impact": "MEDIUM", + "tags": [ + "imports", + "optional-dependencies", + "packaging" + ], + "incorrect": "import anthropic\n\nclass AnthropicProvider:\n ...", + "correct": "try:\n import anthropic\nexcept ImportError as e:\n raise ImportError(\n \"anthropic is required for AnthropicProvider. \"\n \"Install with: pip install 'mylib[anthropic]'\"\n ) from e\n\nclass AnthropicProvider:\n ...", + "explanation": "When a package has optional integrations (e.g., Anthropic support in a multi-provider library), importing the module should not require every optional dep. Handle `ImportError` at module scope with a helpful message pointing to the install extra." + }, + { + "rule": "imports-remove-unused", + "title": "Remove Unused Imports", + "impact": "LOW-MEDIUM", + "tags": [ + "imports", + "dead-code", + "cleanup" + ], + "incorrect": "import json\nimport re\nfrom typing import Any, Optional, Union\n\nfrom .helpers import validate, format_date # format_date never used\n\ndef compact(data: dict[str, Any]) -> str:\n return json.dumps(data, separators=(\",\", \":\"))", + "correct": "import json\nfrom typing import Any\n\n\ndef compact(data: dict[str, Any]) -> str:\n return json.dumps(data, separators=(\",\", \":\"))", + "explanation": "Every import is a declaration of \"this module depends on X.\" Unused imports lie about dependencies, add reading noise, risk circular imports, and can mask refactoring errors (the import survives long after the only call site was deleted)." + }, + { + "rule": "imports-scope-helpers-to-usage", + "title": "Scope Helpers and Constants to Their Usage Site", + "impact": "LOW-MEDIUM", + "tags": [ + "structure", + "scope", + "helpers" + ], + "incorrect": "# somewhere in a 500-line module\ndef _normalize_whitespace(text: str) -> str:\n return \" \".join(text.split())\n\ndef _DEFAULT_MAX_LENGTH() -> int:\n return 280\n\ndef summarize(text: str) -> str:\n text = _normalize_whitespace(text)\n return text[: _DEFAULT_MAX_LENGTH()]\n\n# ... 400 more lines, no other use of _normalize_whitespace or _DEFAULT_MAX_LENGTH", + "correct": "def summarize(text: str) -> str:\n DEFAULT_MAX_LENGTH = 280\n normalized = \" \".join(text.split())\n return normalized[:DEFAULT_MAX_LENGTH]", + "explanation": "When a helper function or constant is only used in one function or class, define it there \u2014 not at module level \"just in case\" someone else needs it later. Module-level scope is a commitment to every future reader: \"this is part of the module's surface.\"" + }, + { + "rule": "imports-top-of-file", + "title": "Place Imports at the Top of the File", + "impact": "LOW-MEDIUM", + "tags": [ + "imports", + "structure", + "conventions" + ], + "incorrect": "def fetch_user(user_id: str) -> User:\n import requests # hidden dependency\n response = requests.get(f\"/users/{user_id}\")\n return User(**response.json())\n\ndef process():\n from .helpers import validate # easily missed\n ...\n import json # another one\n data = json.dumps(result)", + "correct": "import json\nfrom typing import Any\n\nimport requests\n\nfrom .helpers import validate\n\n\ndef fetch_user(user_id: str) -> User:\n response = requests.get(f\"/users/{user_id}\")\n return User(**response.json())\n\ndef process():\n ...", + "explanation": "Imports belong at the top of the module, in conventional ordering (stdlib, third-party, local). Inline imports inside functions hide dependencies from readers, complicate static analysis, and surprise anyone debugging a `ModuleNotFoundError` raised in the middle of a call." + }, + { + "rule": "naming-consistent-terminology", + "title": "Use Consistent Terminology Across Code and Docs", + "impact": "MEDIUM", + "tags": [ + "naming", + "documentation", + "terminology" + ], + "incorrect": "# module_a.py\ndef get_last_message(session): ...\n\n# module_b.py\ndef fetch_latest(session): ...\n\n# module_c.py\ndef current_message(session): ...\n\n# error message\nraise ValueError(\"no recent message found\")", + "correct": "# everywhere\ndef get_latest_message(session): ...\nraise ValueError(\"no latest message found\")\n# docs: \"The latest message is...\"", + "explanation": "When the same concept appears as `message` in one module, `last_message` in another, and `latest` in a third, readers can't grep. Pick one term per concept and use it everywhere \u2014 in code, docstrings, error messages, and external docs." + }, + { + "rule": "naming-drop-redundant-prefixes", + "title": "Drop Redundant Prefixes When Context Is Clear", + "impact": "MEDIUM", + "tags": [ + "naming", + "prefixes", + "conventions" + ], + "incorrect": "from dataclasses import dataclass\n\n@dataclass\nclass ToolConfig:\n tool_name: str\n tool_description: str\n tool_version: str\n tool_timeout: float", + "correct": "@dataclass\nclass ToolConfig:\n name: str\n description: str\n version: str\n timeout: float", + "explanation": "When a field is accessed as `tool_config.tool_description`, the `tool_` prefix adds nothing \u2014 the class name already provides that context. Agents tend to repeat the class name in every field (\"just to be clear\") \u2014 the result is noise that makes real information harder to find." + }, + { + "rule": "naming-no-type-suffixes", + "title": "Avoid Redundant Type Suffixes in Names", + "impact": "LOW-MEDIUM", + "tags": [ + "naming", + "types", + "conventions" + ], + "incorrect": "def filter_users(user_list: list[User], active_dict: dict[str, bool]) -> list[User]:\n name_str = user_list[0].name_str\n result_list: list[User] = []\n ...", + "correct": "def filter_users(users: list[User], active_by_id: dict[str, bool]) -> list[User]:\n name = users[0].name\n result: list[User] = []\n ...", + "explanation": "`user_list: list[User]`, `config_dict: dict[str, str]`, `name_str: str` \u2014 the suffix repeats what the type annotation already says. Python has type annotations; let them do the work. Agents default to Hungarian-style naming because \"it makes the type clear\" \u2014 the type is right there." + }, + { + "rule": "naming-rename-on-behavior-change", + "title": "Rename When Behavior Changes", + "impact": "MEDIUM", + "tags": [ + "naming", + "refactoring", + "honesty" + ], + "incorrect": "# v1: called only for function tools\ndef _call_function_tool(tool: FunctionTool, args: dict) -> Result:\n return tool.invoke(args)\n\n# v2: extended to handle output tools too, but name unchanged\ndef _call_function_tool(tool: FunctionTool | OutputTool, args: dict) -> Result:\n if isinstance(tool, FunctionTool):\n return tool.invoke(args)\n return tool.build_output(args) # wait, this isn't a \"function tool\"", + "correct": "def _call_tool(tool: FunctionTool | OutputTool, args: dict) -> Result:\n if isinstance(tool, FunctionTool):\n return tool.invoke(args)\n return tool.build_output(args)", + "explanation": "A function's name is a promise about what it does. When the behavior changes \u2014 wider scope, different return type, side effects added \u2014 the old name lies. Agents tend to keep names stable because \"it's a smaller diff\"; the cost is that every reader now has to figure out that the name is wrong." + }, + { + "rule": "naming-specific-over-generic", + "title": "Use Specific Parameter and Variable Names", + "impact": "MEDIUM", + "tags": [ + "naming", + "parameters", + "variables" + ], + "incorrect": "def transfer(id: str, id2: str, data: dict, info: dict) -> None:\n ...\n\ndef process_tools(id: str) -> None:\n config = load_config(id)\n memory = load_memory(id) # is this the same id, or a different one?\n ...", + "correct": "def transfer(sender_id: str, recipient_id: str, transfer_data: TransferRequest, audit_info: AuditContext) -> None:\n ...\n\ndef process_tools(toolset_id: str) -> None:\n config = load_config(toolset_id)\n memory = load_memory(toolset_id)\n ...", + "explanation": "Generic names like `id`, `name`, `data`, `info` communicate nothing about what the value is. When you have multiple IDs or data objects in scope, they collide. Prefer names that convey the semantic role." + }, + { + "rule": "naming-upper-case-constants", + "title": "Use UPPER_CASE for Module Constants", + "impact": "LOW-MEDIUM", + "tags": [ + "naming", + "constants", + "conventions" + ], + "incorrect": "# mymodule.py\ndefault_timeout = 30\nmax_retries = 3\nallowed_hosts = frozenset({\"localhost\", \"127.0.0.1\"})\n\ndef fetch(url: str) -> Response:\n for attempt in range(max_retries):\n try:\n return http.get(url, timeout=default_timeout)\n except TimeoutError:\n continue", + "correct": "# mymodule.py\nDEFAULT_TIMEOUT = 30\nMAX_RETRIES = 3\nALLOWED_HOSTS = frozenset({\"localhost\", \"127.0.0.1\"})\n\ndef fetch(url: str) -> Response:\n for attempt in range(MAX_RETRIES):\n try:\n return http.get(url, timeout=DEFAULT_TIMEOUT)\n except TimeoutError:\n continue", + "explanation": "Module-level values that don't change during execution are constants. The `UPPER_CASE` convention signals \"don't reassign this\" and is widely recognized across Python codebases. Agents often leave constants as regular `lower_case` \u2014 the convention is cheap and the signal is strong." + }, + { + "rule": "perf-combine-iterations", + "title": "Combine Filter and Map Into One Pass", + "impact": "LOW-MEDIUM", + "tags": [ + "perf", + "iteration", + "comprehensions" + ], + "incorrect": "def prices_for_sale_items(items: list[Item]) -> list[Decimal]:\n sale_items = [i for i in items if i.on_sale]\n discounted = [i for i in sale_items if i.discount > 0]\n prices = [i.price * (1 - i.discount) for i in discounted]\n return prices", + "correct": "def prices_for_sale_items(items: list[Item]) -> list[Decimal]:\n return [\n item.price * (1 - item.discount)\n for item in items\n if item.on_sale and item.discount > 0\n ]", + "explanation": "When you filter a collection and then map (or map and filter, etc.), it's often one comprehension, not two or three chained operations. Each chained step allocates an intermediate list and iterates." + }, + { + "rule": "perf-compile-regex-module-level", + "title": "Compile Static Regex Patterns at Module Level", + "impact": "MEDIUM", + "tags": [ + "perf", + "regex", + "module-level" + ], + "incorrect": "import re\n\ndef extract_version(text: str) -> str | None:\n match = re.search(r\"v(\\d+\\.\\d+\\.\\d+)\", text) # compiled every call\n return match.group(1) if match else None", + "correct": "import re\n\n_VERSION_RE = re.compile(r\"v(\\d+\\.\\d+\\.\\d+)\")\n\ndef extract_version(text: str) -> str | None:\n match = _VERSION_RE.search(text)\n return match.group(1) if match else None", + "explanation": "`re.compile()` builds a pattern object once; `re.match()` / `re.search()` on a string call it every time. For regexes that don't change, compile at module scope. The cost of recompilation in a hot loop can dwarf the actual match." + }, + { + "rule": "perf-dict-index-over-nested-loops", + "title": "Build a Dict Index Instead of Nested Loops", + "impact": "MEDIUM", + "tags": [ + "perf", + "dict", + "index", + "nested-loops" + ], + "incorrect": "def attach_profiles(users: list[User], profiles: list[Profile]) -> list[EnrichedUser]:\n result = []\n for user in users:\n matching = None\n for profile in profiles:\n if profile.user_id == user.id:\n matching = profile\n break\n result.append(EnrichedUser(user=user, profile=matching))\n return result", + "correct": "def attach_profiles(users: list[User], profiles: list[Profile]) -> list[EnrichedUser]:\n profiles_by_user = {p.user_id: p for p in profiles}\n return [\n EnrichedUser(user=user, profile=profiles_by_user.get(user.id))\n for user in users\n ]", + "explanation": "When code says \"for each item in A, find the matching item in B,\" agents default to nested `for` + `if x.id == y.id`. That's O(n \u00d7 m). Build a dict from B once, then it's O(n + m) total with the body of the loop becoming a single lookup." + }, + { + "rule": "perf-generator-over-list", + "title": "Stream with Generators When Memory or First-Result Latency Matters", + "impact": "MEDIUM", + "tags": [ + "perf", + "generators", + "memory", + "streaming" + ], + "incorrect": "def count_errors(path: Path) -> int:\n lines = path.read_text().splitlines() # full file in memory\n parsed = [parse_line(line) for line in lines] # second full copy\n matching = [p for p in parsed if p.level == \"ERROR\"] # third full copy\n return len(matching)", + "correct": "def count_errors(path: Path) -> int:\n with path.open() as f:\n return sum(1 for line in f if parse_line(line).level == \"ERROR\")", + "explanation": "Generators trade materialization for laziness: they yield one value at a time, hold no intermediate list, and let the caller stop early. This is a **memory and streaming** rule, not a \"generators are categorically better than lists\" rule. When you need every result anyway, iterate the sequence more than once, want random access, or want to sort it \u2014 a list comprehension is the right tool, often clearer and sometimes faster (no `next()` overhead per element)." + }, + { + "rule": "perf-isinstance-tuple-syntax", + "title": "Prefer Tuple Syntax in isinstance() Only on Profiled Hot Paths", + "impact": "LOW", + "tags": [ + "perf", + "isinstance", + "micro-optimization", + "hot-path" + ], + "incorrect": "# A drive-by PR that flips every isinstance() in the codebase.\ndef is_numeric(x: object) -> bool:\n return isinstance(x, (int, float)) # was: isinstance(x, int | float)", + "correct": "# This validator runs once per row across ~10M rows in the ETL job \u2014 profiled.\n_PRIMITIVE_TYPES = (int, float, str, bool)\n\ndef is_primitive(x: object) -> bool:\n return isinstance(x, _PRIMITIVE_TYPES)", + "explanation": "Both `isinstance(x, (A, B, C))` and `isinstance(x, A | B | C)` are correct and supported in Python 3.10+. They produce the same result. The tuple form is *marginally* faster on each call because the union form constructs a `types.UnionType` object, but the gap is small enough that it only matters inside loops you've actually profiled. **Do not blanket-rewrite a codebase from union to tuple syntax** \u2014 the noise is rarely worth the diff." + }, + { + "rule": "perf-lru-cache-pure-fns", + "title": "Use functools.lru_cache for Pure Functions", + "impact": "MEDIUM", + "tags": [ + "perf", + "lru-cache", + "caching", + "functools" + ], + "incorrect": "def parse_version(version_str: str) -> Version:\n # called from many call sites, often with the same string\n return Version.parse(version_str)", + "correct": "from functools import lru_cache\n\n@lru_cache(maxsize=256)\ndef parse_version(version_str: str) -> Version:\n return Version.parse(version_str)", + "explanation": "When a function is pure (same input \u2192 same output, no side effects) and called repeatedly with the same arguments, `@lru_cache` caches the result so subsequent calls are free. Agents often forget this exists and either hand-roll a dict cache or eat the recomputation cost." + }, + { + "rule": "perf-set-for-membership", + "title": "Use set for Repeated Membership Checks", + "impact": "MEDIUM", + "tags": [ + "perf", + "set", + "membership", + "data-structures" + ], + "incorrect": "def filter_allowed(items: list[Item], allowed: list[str]) -> list[Item]:\n return [item for item in items if item.id in allowed]", + "correct": "def filter_allowed(items: list[Item], allowed: list[str]) -> list[Item]:\n allowed_set = set(allowed)\n return [item for item in items if item.id in allowed_set]", + "explanation": "`x in some_list` scans the list every time \u2014 O(n). `x in some_set` is a hash lookup \u2014 O(1). When you're checking membership repeatedly against the same collection, the set conversion pays for itself quickly." + }, + { + "rule": "perf-type-adapter-constant", + "title": "Define TypeAdapter Instances at Module Level", + "impact": "MEDIUM", + "tags": [ + "perf", + "pydantic", + "type-adapter", + "module-level", + "applicability:pydantic" + ], + "incorrect": "from pydantic import TypeAdapter\n\ndef parse_users(raw: bytes) -> list[User]:\n adapter = TypeAdapter(list[User]) # schema built every call\n return adapter.validate_json(raw)", + "correct": "from pydantic import TypeAdapter\n\n_USERS_ADAPTER: TypeAdapter[list[User]] = TypeAdapter(list[User])\n\ndef parse_users(raw: bytes) -> list[User]:\n return _USERS_ADAPTER.validate_json(raw)", + "explanation": "" + }, + { + "rule": "simplify-any-all-builtins", + "title": "Use any() / all() Over Boolean-Flag Loops", + "impact": "MEDIUM", + "tags": [ + "simplify", + "builtins", + "any", + "all" + ], + "incorrect": "def has_admin(users: list[User]) -> bool:\n found = False\n for user in users:\n if user.is_admin:\n found = True\n break\n return found\n\ndef all_ready(services: list[Service]) -> bool:\n for s in services:\n if not s.ready:\n return False\n return True", + "correct": "def has_admin(users: list[User]) -> bool:\n return any(u.is_admin for u in users)\n\ndef all_ready(services: list[Service]) -> bool:\n return all(s.ready for s in services)", + "explanation": "When you're checking \"does any element satisfy X?\" or \"do all elements satisfy X?\", Python has built-ins for that. Agents sometimes write manual `found = False` / `break` patterns \u2014 more code, more bugs, no short-circuit benefit." + }, + { + "rule": "simplify-cached-property", + "title": "Use @cached_property Only When the Instance Supports It", + "impact": "MEDIUM", + "tags": [ + "simplify", + "cached-property", + "performance", + "threading" + ], + "incorrect": "from dataclasses import dataclass\n\n@dataclass\nclass Report:\n rows: list[Row]\n\n def summary_stats(self) -> Stats: # expensive, called many times\n return compute_stats(self.rows)", + "correct": "from dataclasses import dataclass, field\nfrom functools import cached_property\n\n@dataclass(frozen=True)\nclass Report:\n rows: tuple[Row, ...] # immutable container; cannot be mutated after construction\n\n @cached_property\n def summary_stats(self) -> Stats:\n return compute_stats(self.rows)", + "explanation": "`@cached_property` is the right tool when the cached value is **derived from effectively immutable inputs**, the getter is **idempotent**, the class **has a writable `__dict__`**, and the instance is not shared across threads racing on first access. Outside that envelope, the convenience masks real bugs: stale caches when inputs mutate, `TypeError` on `__slots__` classes that omit `__dict__`, and duplicated computation when two threads hit the property simultaneously." + }, + { + "rule": "simplify-comprehensions", + "title": "Use Comprehensions Over for+append Loops", + "impact": "MEDIUM-HIGH", + "tags": [ + "simplify", + "comprehensions", + "idioms" + ], + "incorrect": "def active_usernames(users: list[User]) -> list[str]:\n result = []\n for user in users:\n if user.is_active:\n result.append(user.name)\n return result\n\ndef name_to_id(users: list[User]) -> dict[str, str]:\n mapping = {}\n for user in users:\n mapping[user.name] = user.id\n return mapping", + "correct": "def active_usernames(users: list[User]) -> list[str]:\n return [user.name for user in users if user.is_active]\n\ndef name_to_id(users: list[User]) -> dict[str, str]:\n return {user.name: user.id for user in users}", + "explanation": "Comprehensions express \"build a collection from an iterable\" in one line. Agents often write C-style loops with `append()` \u2014 more code, more variables, more places for off-by-one and wrong-list bugs. Reach for a comprehension by default." + }, + { + "rule": "simplify-early-return", + "title": "Return Early to Flatten Control Flow", + "impact": "MEDIUM", + "tags": [ + "simplify", + "control-flow", + "guard-clauses" + ], + "incorrect": "def process_request(req: Request) -> Response:\n if req.authenticated:\n if req.authorized:\n if req.body is not None:\n if req.body.is_valid:\n return do_process(req.body)\n else:\n return error(400, \"invalid body\")\n else:\n return error(400, \"missing body\")\n else:\n return error(403, \"forbidden\")\n else:\n return error(401, \"unauthenticated\")", + "correct": "def process_request(req: Request) -> Response:\n if not req.authenticated:\n return error(401, \"unauthenticated\")\n if not req.authorized:\n return error(403, \"forbidden\")\n if req.body is None:\n return error(400, \"missing body\")\n if not req.body.is_valid:\n return error(400, \"invalid body\")\n\n return do_process(req.body)", + "explanation": "When a function has preconditions to check, return as soon as one fails. Agents tend to write deeply nested \"if valid, if authorized, if ...\" pyramids \u2014 the happy path ends up buried five levels in. Guard clauses flatten the structure and make the happy path the most visible branch." + }, + { + "rule": "simplify-extract-after-duplication", + "title": "Extract Helpers After 2+ Occurrences", + "impact": "MEDIUM-HIGH", + "tags": [ + "simplify", + "extraction", + "dry" + ], + "incorrect": "def handle_stream(stream: AsyncIterator[Chunk]) -> Response:\n collected = []\n async for chunk in stream:\n if chunk.kind == \"text\":\n collected.append(chunk.text)\n elif chunk.kind == \"tool\":\n collected.append(format_tool(chunk))\n return Response(content=\"\".join(collected))\n\ndef handle_non_stream(response: RawResponse) -> Response:\n collected = []\n for chunk in response.chunks:\n if chunk.kind == \"text\":\n collected.append(chunk.text)\n elif chunk.kind == \"tool\":\n collected.append(format_tool(chunk))\n return Response(content=\"\".join(collected))", + "correct": "def _format_chunk(chunk: Chunk) -> str:\n match chunk.kind:\n case \"text\": return chunk.text\n case \"tool\": return format_tool(chunk)\n case _: return \"\"\n\nasync def handle_stream(stream: AsyncIterator[Chunk]) -> Response:\n parts = [_format_chunk(c) async for c in stream]\n return Response(content=\"\".join(parts))\n\ndef handle_non_stream(response: RawResponse) -> Response:\n parts = [_format_chunk(c) for c in response.chunks]\n return Response(content=\"\".join(parts))", + "explanation": "The first copy of a piece of logic is fine. The second copy is the point of decision: extract now, or accept drift later. Agents tend to copy-paste a third time because \"extracting is a bigger change\"; the cost is bugs where two copies evolved in subtly different directions." + }, + { + "rule": "simplify-fallback-or", + "title": "Use x or default for Fallback Values", + "impact": "LOW-MEDIUM", + "tags": [ + "simplify", + "fallback", + "or" + ], + "incorrect": "def display_name(user: User) -> str:\n if user.nickname:\n return user.nickname\n else:\n return user.username", + "correct": "def display_name(user: User) -> str:\n return user.nickname or user.username", + "explanation": "For the common \"use `x` if it's truthy, otherwise `default`\" pattern, `x or default` beats the verbose `if`/`else`. The catch: this triggers on every falsy value (`0`, `''`, `[]`, `None`) \u2014 so only use it when those aren't semantically meaningful." + }, + { + "rule": "simplify-flatten-nested-if", + "title": "Flatten Nested if Statements Into and Conditions", + "impact": "LOW-MEDIUM", + "tags": [ + "simplify", + "control-flow", + "conditionals" + ], + "incorrect": "def should_notify(user: User, event: Event) -> bool:\n if user.is_active:\n if user.notifications_enabled:\n if event.priority >= user.notification_threshold:\n return True\n return False", + "correct": "def should_notify(user: User, event: Event) -> bool:\n return (\n user.is_active\n and user.notifications_enabled\n and event.priority >= user.notification_threshold\n )", + "explanation": "When nested `if` statements share the same body and have no intervening code, collapse them into a single `if` with `and`. The nested form implies the branches do something different; when they don't, the structure lies." + }, + { + "rule": "simplify-inline-single-use-vars", + "title": "Inline Single-Use Intermediate Variables", + "impact": "LOW-MEDIUM", + "tags": [ + "simplify", + "variables", + "inline" + ], + "incorrect": "def top_admins(users: list[User], limit: int) -> list[User]:\n filtered_users = [u for u in users if u.is_admin]\n sorted_users = sorted(filtered_users, key=lambda u: u.rank)\n result = sorted_users[:limit]\n return result", + "correct": "def top_admins(users: list[User], limit: int) -> list[User]:\n return sorted(\n (u for u in users if u.is_admin),\n key=lambda u: u.rank,\n )[:limit]", + "explanation": "When a variable is assigned once and used once immediately after, inlining it removes a name that doesn't earn its keep. Agents tend to introduce `_filtered`, `_cleaned`, `_copy` intermediates \"for clarity\" \u2014 but the clarity is usually from the name, and if the name isn't informative, the variable is just noise." + }, + { + "rule": "simplify-remove-dead-code", + "title": "Remove Commented-Out and Dead Code", + "impact": "MEDIUM", + "tags": [ + "simplify", + "dead-code", + "cleanup" + ], + "incorrect": "def fetch_user(user_id: str) -> User:\n # Old implementation:\n # response = requests.get(f\"/users/{user_id}\")\n # return User(**response.json())\n\n response = http_client.get(f\"/users/{user_id}\")\n return User.model_validate(response.json())\n\n # TODO: switch to async version once available\n # async def fetch_user(user_id): ...\n\ndef _unused_helper(x: int) -> int: # nothing calls this\n return x * 2", + "correct": "def fetch_user(user_id: str) -> User:\n response = http_client.get(f\"/users/{user_id}\")\n return User.model_validate(response.json())", + "explanation": "Commented-out code, superseded implementations, unused imports, and definitions nothing calls \u2014 delete them. Version control preserves history; dead code in the file confuses readers about which implementation is actually active." + }, + { + "rule": "types-avoid-any", + "title": "Avoid Any Annotations", + "impact": "CRITICAL", + "tags": [ + "types", + "any", + "precision", + "protocols" + ], + "incorrect": "from typing import Any\n\ndef process_items(items: Any) -> Any:\n return [transform(item) for item in items]\n\ndef transform(item: Any) -> Any:\n return item.value.upper()", + "correct": "from typing import Protocol\n\nclass HasValue(Protocol):\n value: str\n\ndef process_items(items: list[HasValue]) -> list[str]:\n return [transform(item) for item in items]\n\ndef transform(item: HasValue) -> str:\n return item.value.upper()", + "explanation": "`Any` turns off the type checker for that value \u2014 it accepts anything, produces anything, and propagates silently into every call site that consumes it. Agents reach for `Any` when the right type feels hard; almost always, a `Protocol`, `TypeVar`, or `Union` is available." + }, + { + "rule": "types-fix-errors-not-ignore", + "title": "Fix Type Errors, Don't Ignore Them", + "impact": "HIGH", + "tags": [ + "types", + "mypy", + "pyright", + "ignore" + ], + "incorrect": "def compute(items: list[int] | None) -> int:\n return sum(items) # type: ignore # noqa", + "correct": "def compute(items: list[int] | None) -> int:\n if items is None:\n return 0\n return sum(items)", + "explanation": "`# type: ignore` and `# pyright: ignore` silence the checker \u2014 but the underlying problem stays. Agents reach for ignore comments when a type looks hard; each one degrades the signal from every future run. Fix the error properly, and when a suppression is genuinely unavoidable, document why." + }, + { + "rule": "types-fix-types-not-cast", + "title": "Fix Type Definitions Instead of cast()", + "impact": "HIGH", + "tags": [ + "types", + "cast", + "design" + ], + "incorrect": "from typing import cast\n\ndef load_config() -> dict[str, object]:\n return json.loads(CONFIG_PATH.read_text())\n\ndef get_timeout() -> int:\n config = load_config()\n return cast(int, config[\"timeout\"]) # we're just telling the checker to trust us", + "correct": "from typing import TypedDict\n\nclass Config(TypedDict):\n timeout: int\n retries: int\n\ndef load_config() -> Config:\n return json.loads(CONFIG_PATH.read_text()) # validate or cast here, once\n\ndef get_timeout() -> int:\n config = load_config()\n return config[\"timeout\"] # known to be int from Config", + "explanation": "`cast(T, value)` tells the checker to pretend `value` is a `T` with no runtime check. When called to paper over a structural mismatch, it hides a design problem. Reach for it only when runtime logic genuinely narrows in a way the checker can't express." + }, + { + "rule": "types-isinstance-for-narrowing", + "title": "Use isinstance() for Type Checking, Not hasattr/getattr", + "impact": "CRITICAL", + "tags": [ + "types", + "isinstance", + "narrowing", + "duck-typing" + ], + "incorrect": "def process(part: MessagePart) -> str:\n if hasattr(part, \"tool_name\"):\n return f\"Tool: {part.tool_name}\" # type checker: attribute is Any\n if getattr(part, \"kind\", None) == \"text\":\n return part.text # type checker: does part.text exist? unclear\n if type(part).__name__ == \"ImagePart\":\n return f\"Image: {part.url}\" # fragile: renaming ImagePart breaks this\n return \"unknown\"", + "correct": "def process(part: MessagePart) -> str:\n if isinstance(part, ToolPart):\n return f\"Tool: {part.tool_name}\" # narrowed to ToolPart\n if isinstance(part, TextPart):\n return part.text # narrowed to TextPart\n if isinstance(part, ImagePart):\n return f\"Image: {part.url}\" # narrowed to ImagePart\n return \"unknown\"", + "explanation": "Type checkers narrow types through `isinstance()` checks, discriminator match statements, and `TypeGuard` functions \u2014 not through `hasattr()`, `getattr()`, or `type(obj).__name__ == \"...\"`. Agents reach for `hasattr` for \"flexibility\"; the actual cost is that the checker can't narrow and refactors silently break string comparisons." + }, + { + "rule": "types-literal-for-fixed-sets", + "title": "Use Literal Types for Fixed String Sets", + "impact": "HIGH", + "tags": [ + "types", + "literal", + "strings", + "validation" + ], + "incorrect": "def set_log_level(level: str) -> None:\n ...\n\nset_log_level(\"DEUBG\") # typo \u2014 compiles fine, runtime surprise", + "correct": "from typing import Literal\n\nLogLevel = Literal[\"DEBUG\", \"INFO\", \"WARNING\", \"ERROR\"]\n\ndef set_log_level(level: LogLevel) -> None:\n ...\n\nset_log_level(\"DEUBG\") # type error \u2014 caught at type-check time", + "explanation": "When a parameter accepts one of a fixed set of string values, `str` is too wide \u2014 every typo is legal. `Literal[\"a\", \"b\", \"c\"]` narrows the type to exactly those values and enables `match` exhaustiveness checking." + }, + { + "rule": "types-narrow-to-runtime-reality", + "title": "Narrow Type Signatures to Runtime Reality", + "impact": "MEDIUM", + "tags": [ + "types", + "narrowing", + "unions", + "design" + ], + "incorrect": "def render_tool_result(part: MessagePart) -> str:\n # by contract this is only called with ToolResultPart or ToolCallPart\n if isinstance(part, ToolResultPart):\n return f\"Result: {part.content}\"\n if isinstance(part, ToolCallPart):\n return f\"Call: {part.tool_name}\"\n if isinstance(part, TextPart):\n return part.text # unreachable \u2014 caller never passes TextPart\n raise ValueError(f\"unexpected part: {part}\")", + "correct": "ToolPart = ToolCallPart | ToolResultPart\n\ndef render_tool_result(part: ToolPart) -> str:\n if isinstance(part, ToolResultPart):\n return f\"Result: {part.content}\"\n return f\"Call: {part.tool_name}\" # must be ToolCallPart", + "explanation": "If control flow (a `match` statement, an API contract, an earlier `isinstance` check) guarantees that only a subset of a union reaches a code path, the annotation should reflect that \u2014 not the wider union. Over-broad annotations create dead branches and suggest possibilities the code can't actually handle." + }, + { + "rule": "types-remove-redundant-optional", + "title": "Remove Redundant `| None` When Values Are Guaranteed", + "impact": "MEDIUM", + "tags": [ + "types", + "optional", + "none", + "annotations" + ], + "incorrect": "from dataclasses import dataclass\n\n@dataclass\nclass Session:\n user_id: str\n token: str | None = None # but we always generate a token in __post_init__\n\n def __post_init__(self) -> None:\n if self.token is None:\n self.token = generate_token()", + "correct": "from dataclasses import dataclass, field\n\n@dataclass\nclass Session:\n user_id: str\n token: str = field(default_factory=generate_token)", + "explanation": "An annotation of `X | None` tells readers and the checker that `None` is a real possibility \u2014 every consumer now writes a `None` check. When the value is guaranteed to be set (by the constructor, by the control flow, by an earlier validation), `| None` lies about the API." + }, + { + "rule": "types-trust-the-checker", + "title": "Trust the Type Checker \u2014 Remove Redundant Runtime Checks", + "impact": "MEDIUM", + "tags": [ + "types", + "runtime-checks", + "assertions" + ], + "incorrect": "def process_user(user: User) -> str:\n assert user is not None # type says User, not User | None\n assert isinstance(user, User) # type already says User\n assert user.name # if name: str, this only catches empty strings\n return user.name.upper()", + "correct": "def process_user(user: User) -> str:\n return user.name.upper()", + "explanation": "When types already constrain a value, runtime checks for the same constraint add noise and imply the types aren't trustworthy. Every redundant `assert` or `isinstance` is a vote of no confidence in the rest of the type system." + }, + { + "rule": "types-type-checking-imports", + "title": "Use TYPE_CHECKING for Optional Dependencies", + "impact": "MEDIUM", + "tags": [ + "types", + "imports", + "optional-dependencies", + "typing" + ], + "incorrect": "import anthropic # crashes if anthropic is not installed\n\nclass AnthropicProvider:\n def __init__(self, client: anthropic.Client) -> None:\n self._client = client", + "correct": "from __future__ import annotations\n\nfrom typing import TYPE_CHECKING\n\nif TYPE_CHECKING:\n import anthropic\n\nclass AnthropicProvider:\n def __init__(self, client: anthropic.Client) -> None:\n self._client = client", + "explanation": "When a module's type hints reference a class from an optional dependency, importing the module should not require that dependency to be installed. `if TYPE_CHECKING:` blocks let the checker see the import while the runtime stays lean." + }, + { + "rule": "types-typeddict-over-dict-any", + "title": "Use TypedDict or Dataclass Instead of dict[str, Any]", + "impact": "CRITICAL", + "tags": [ + "types", + "typeddict", + "dataclass", + "any" + ], + "incorrect": "from typing import Any\n\ndef create_user(config: dict[str, Any]) -> User:\n name = config[\"name\"] # what type?\n age = config.get(\"age\", 0) # what type? what's the default type?\n prefs = config.get(\"prefs\", {}) # dict or None or the passed-in value?\n return User(name=name.upper(), age=age + 1, prefs=prefs)", + "correct": "from typing import TypedDict, NotRequired\n\nclass UserPreferences(TypedDict):\n theme: NotRequired[str]\n notifications: NotRequired[bool]\n\nclass UserConfig(TypedDict):\n name: str\n age: NotRequired[int]\n prefs: NotRequired[UserPreferences]\n\ndef create_user(config: UserConfig) -> User:\n name = config[\"name\"] # str\n age = config.get(\"age\", 0) # int\n prefs = config.get(\"prefs\", {}) # UserPreferences\n return User(name=name.upper(), age=age + 1, prefs=prefs)", + "explanation": "When the shape of a dict is known (config objects, API payloads, structured event data), `dict[str, Any]` is a lie \u2014 the structure exists, it's just not declared. Every access becomes a runtime gamble. `TypedDict` or `dataclass` restores type-checker coverage." + } + ] +}