Python Best Practices

Python Best Practices

Guidelines for writing and reviewing Python. 70 rules across 8 categories, prioritized by impact.

A rule match is a signal, not a verdict. Most rules are design preferences for new code, not bugs to fix across the repo — check the rule's impact level before flagging in review or refactoring stable code.

When to Apply

• Writing new Python modules, functions, classes, or data models
• Reviewing code for correctness or type safety
• Refactoring patterns in code that's being edited anyway

Avoid applying these rules as a blanket sweep across stable code — the churn rarely pays off.

Impact Levels

• CRITICAL — prevents a real bug class (data corruption, swallowed cancellations, insecure defaults). Fix when found.
• HIGH — meaningful correctness or maintainability win. Worth fixing in most contexts.
• MEDIUM — good practice; clarity or drift prevention. Apply to new code; don't churn stable code.
• LOW-MEDIUM / LOW — style or micro-optimizations. Apply opportunistically.

Python Version Baseline

Rules assume Python 3.11+. Rules depending on higher versions call it out inline:

• warnings.deprecated() — 3.13+
• zoneinfo — 3.9+
• Union types in isinstance() — 3.10+
• assert_never — 3.11+ (backport via typing_extensions)

Rules tagged applicability:pydantic are Pydantic-specific.

Rule Categories by Priority

Section impact is a typical-case label; individual rules range one level above or below — check the rule file.

Quick Reference

Data Modeling (data-)

• data-mutable-defaults — Never def f(items=[]); use None + body construction or default_factory
• data-derive-dont-store — Compute booleans from state; don't cache flags that mirror each other
• data-mutation-contract — Mutate OR return; not both
• data-aware-datetimes — Timezone-aware datetime.now(timezone.utc); utcnow() is deprecated
• data-discriminated-unions — Tag variants instead of optional-field bags
• data-explicit-variants — Concrete classes per mode beat is_thread / is_edit flags
• data-phased-composition — Group co-present optionals into one nested optional
• data-encapsulate-mutable-state — Trap mutable state in the narrowest clear scope
• data-sentinel-when-none-is-valid — Private sentinel when None is a meaningful value
• data-newtype-for-ids — NewType('UserId', str) so IDs aren't interchangeable
• data-delete-dead-variants — Remove union arms that aren't constructed

Error Handling (error-)

• error-specific-exceptions — Catch specific types; never bare except: or except BaseException: (breaks Ctrl-C and async cancellation); except Exception: is cancellation-safe on 3.8+
• error-context-managers — with / async with for files, locks, sessions
• error-assert-debug-only — assert vanishes under -O; not for runtime contracts
• error-validate-at-boundaries — Fail fast at system edges before expensive work
• error-trust-validated-state — Trust immutable, locally-constructed state
• error-consolidate-try-except — Merge blocks with the same catch and handling
• error-assert-never-exhaustiveness — typing.assert_never for exhaustiveness
• error-raise-from-for-chains — raise NewErr(...) from original to preserve causality
• error-inherit-base-exceptions — New exceptions inherit existing bases for compatibility
• error-log-exception-context — logger.exception(...) inside except; keep the traceback in the log
• error-repr-in-messages — f"tool {name!r}" for identifiers in error text

Type Safety (types-)

• types-fix-errors-not-ignore — Fix type errors; # type: ignore is a last resort
• types-avoid-any — Protocols, TypeVars, unions over Any
• types-typeddict-over-dict-any — TypedDict / dataclass when structure is known
• types-literal-for-fixed-sets — Literal["a", "b"] for fixed strings
• types-fix-types-not-cast — Fix the definition; cast() only when runtime genuinely narrows
• types-isinstance-for-narrowing — isinstance() over hasattr / type(x).__name__
• types-narrow-to-runtime-reality — Annotations match what control flow actually allows
• types-trust-the-checker — Drop runtime checks the types already enforce
• types-remove-redundant-optional — Drop | None when values are guaranteed present
• types-type-checking-imports — if TYPE_CHECKING: for optional or heavy imports

API Design (api-)

• api-required-before-optional — Required fields before optional (Python enforces this)
• api-keyword-only-params — * marker for optional/config params
• api-no-boolean-flag-params — Literal / Enum over True, False soup
• api-immutable-transforms — Return new collections; don't mutate inputs
• api-model-cohesion — Flat models; no duplicate or single-key-wrapped fields
• api-underscore-for-private — _prefix for internals; exclude from __all__
• api-deprecated-aliases — warnings.deprecated() (3.13+) for renamed APIs
• api-no-private-access — Don't reach into _prefixed names from outside the module
• api-instance-vs-module-fn — Pick the namespace that matches ownership

Code Simplification (simplify-)

• simplify-early-return — Return early; don't nest the happy path
• simplify-extract-after-duplication — Second copy is the decision point; third is the safe default
• simplify-cached-property — @cached_property on immutable instances; not thread-safe
• simplify-comprehensions — Comprehensions over for + .append()
• simplify-any-all-builtins — any() / all() over manual flag + break
• simplify-fallback-or — x or default when falsy values aren't semantic
• simplify-flatten-nested-if — if cond1 and cond2: when no intervening code
• simplify-inline-single-use-vars — Drop intermediates used once
• simplify-remove-dead-code — Delete commented-out code; git preserves history

Performance (perf-)

• perf-set-for-membership — set for repeated in checks
• perf-dict-index-over-nested-loops — Build a dict for lookups
• perf-lru-cache-pure-fns — functools.lru_cache / functools.cache on pure functions
• perf-generator-over-list — Stream with generators when memory or latency matters
• perf-combine-iterations — Fuse filter + map into one pass
• perf-compile-regex-module-level — Compile static regex at module scope; matters in tight loops
• perf-type-adapter-constant — Module-scope TypeAdapter (applicability: pydantic)
• perf-isinstance-tuple-syntax — Tuple form is marginally faster; profiled hot paths only

Naming (naming-)

• naming-rename-on-behavior-change — Rename when behavior changes; stale names mislead
• naming-consistent-terminology — Same concept, same word across code/docs/errors
• naming-specific-over-generic — toolset_id; not bare id
• naming-drop-redundant-prefixes — ToolConfig.description; not ToolConfig.tool_description
• naming-upper-case-constants — MAX_RETRIES; _ prefix for internal
• naming-no-type-suffixes — No _dict / _list suffixes; types annotate types

Imports & Structure (imports-)

• imports-no-side-effects — Modules must be cheap to import — no network/model/env reads at import
• imports-top-of-file — Imports at the top; documented exceptions for circular / optional / deferred
• imports-optional-dependencies — try / except ImportError with install hints
• imports-scope-helpers-to-usage — Define helpers near where they're used
• imports-remove-unused — Delete unused imports
• imports-no-duplicates — One import per name

How to Use

Read individual rule files for detail:

rules/data-mutable-defaults.md
rules/error-specific-exceptions.md

Each rule has:

• Impact level in frontmatter
• Brief explanation
• Incorrect example
• Correct example
• Optional note on edge cases

For the full compiled guide with all rules expanded: AGENTS.md.

Python Best Practices

A structured skill for writing and reviewing Python. Rules are derived from real PR review patterns, organized by impact and applicability, and formatted for agent consumption.

Python version baseline: 3.11+ (some rules call out higher-version features inline — e.g., warnings.deprecated() is 3.13+).

Structure

python-best-practices/
├── SKILL.md                # Entrypoint loaded into agent context (quick reference)
├── README.md               # This file
├── metadata.json           # Version, abstract, references, Python version floor
├── AGENTS.md               # (generated) Compiled document with every rule expanded
├── test-cases.json         # (generated) LLM eval data extracted from rule examples
├── rules/                  # Individual rule files (one rule per file)
│   ├── _sections.md        # Section metadata
│   ├── _template.md        # Template for new rules
│   └── {prefix}-{name}.md  # Rule files; `prefix` matches a section in `_sections.md`
└── src/                    # Build, validate, extract-tests scripts

Sections

Section impact is the typical case; individual rules range one level above or below — always check the rule frontmatter. Applicability (e.g., Pydantic-only) is tagged on the rule, not the section.

Impact Levels

• CRITICAL — prevents a real bug class (data corruption, swallowed cancellations, insecure defaults)
• HIGH — meaningful correctness or maintainability win
• MEDIUM-HIGH — noticeable improvement worth enforcing
• MEDIUM — good practice; clarity or drift prevention
• LOW-MEDIUM — marginal; new code preferred
• LOW — style; opportunistic only

Reserve CRITICAL for bug classes you'd block a PR on. If more than a handful of rules are CRITICAL, the signal is lost.

Authoring Workflow

1. Copy rules/_template.md to rules/{prefix}-{name}.md 2. Choose the prefix from _sections.md 3. Fill in frontmatter (title, impact, impactDescription, tags, references) 4. Write a short explanation + Incorrect/Correct pair + optional note 5. Run src/validate.py → fix → src/build.py → src/extract_tests.py

Keep rule bodies short — target 20–40 lines. One Incorrect block, one Correct block, optional one-paragraph note. Avoid enumerated "use X when / use Y when" taxonomies; let the example carry the point.

Scripts

python src/build.py            # compile rules into AGENTS.md
python src/validate.py         # lint frontmatter, references, example structure
python src/extract_tests.py    # generate test-cases.json for LLM evals

Typical loop: validate.py → fix → build.py → extract_tests.py before commit. AGENTS.md and test-cases.json are generated outputs — don't hand-edit.

Rule File Format

---
title: Rule Title Here
impact: MEDIUM
impactDescription: brief phrase on the payoff
tags: tag1, tag2
references: https://docs.python.org/3/library/...
---

## Rule Title Here

Brief explanation — one or two sentences. Observational, not prescriptive.

**Incorrect:**

Bad example

**Correct:**

Good example

Optional one-paragraph note on edge cases or version notes.

When references is required

references is required when the rule depends on:

• A specific Python version (3.10 union isinstance, 3.11 assert_never, 3.13 warnings.deprecated)
• Standard-library behavior with versioned semantics (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.

Tagging applicability

Rules that only apply within a specific ecosystem (e.g., Pydantic) carry an applicability:{name} tag in tags and call it out in the body.

Tone

Rules are observational, not prescriptive. Describe the pattern and the cost; show the fix. Don't moralize about what an agent "is tempted to do" — the reader already has the code in front of them.

Rule bodies should leave judgment to the reader. A rule that applies everywhere in a codebase without exception is rare; most need a reader who knows their context.

Acknowledgments

Rule material draws from PR-review patterns in the Pydantic AI codebase, Vercel's react-best-practices and composition-patterns skills (structural inspiration and tone), and the clanker-discipline skill.

Sections

This file defines all sections, their ordering, impact levels, and descriptions. The section ID (in parentheses) is the filename prefix used to group rules.

Section impact is a typical-case label. Individual rules range one level above or below the section — check each rule's frontmatter.

---

1. Data Modeling (data)

Impact: HIGH Description: The architectural foundation. Mutable defaults, mutation contracts, timezone-aware datetimes, discriminated unions. Mistakes here compound into state nobody intended.

2. Error Handling (error)

Impact: MEDIUM-HIGH Description: Specific exceptions, context managers for resources, preserved cancellation semantics. Sloppy exceptions hide bugs; narrow catches localize them.

3. Type Safety (types)

Impact: MEDIUM-HIGH Description: Precise types catch bugs at type-check time. Fix type errors rather than ignore them; no Any drift in new code.

4. API Design (api)

Impact: MEDIUM Description: Interfaces that age well. Required-before-optional ordering, keyword-only params, no boolean flag soup. Mostly applies to new code.

5. Code Simplification (simplify)

Impact: LOW-MEDIUM Description: Python idioms. Comprehensions, any()/all(), early returns. Mostly stylistic — apply when writing or reviewing.

6. Performance (perf)

Impact: LOW-MEDIUM Description: Python-specific optimizations. Set/dict lookups, cached properties, module-level compilation. Applied on measured hot paths, not as a stylistic crusade.

7. Naming (naming)

Impact: LOW-MEDIUM Description: Names carry meaning. Specific over generic, consistent terminology, no type suffixes. Mostly applies to new names.

8. Imports & Structure (imports)

Impact: LOW Description: Module hygiene. Imports at the top, no import-time side effects, optional deps handled explicitly. Most items linters catch automatically.

Rule Title Here

Brief explanation — one or two sentences. Observational: describe the pattern and what it costs. Avoid "the impulse to avoid" or "the temptation of" framing.

Incorrect:

# Bad code example

Correct:

# Good code example

Optional one-paragraph note on nuance, edge cases, or version-specific behavior. Keep it short. Skip if the examples carry the point.

---

Authoring notes

Target body length: 20–40 lines. One Incorrect block, one Correct block, optional note. Cut enumerated "use X when / use Y when" taxonomies — let the example speak.

Impact

• CRITICAL — prevents a real bug class (data corruption, swallowed cancellations, insecure defaults)
• HIGH — meaningful correctness or maintainability win
• MEDIUM-HIGH — noticeable improvement worth enforcing
• MEDIUM — good practice; clarity or drift prevention
• LOW-MEDIUM / LOW — style; opportunistic

Reserve CRITICAL for bug classes you'd block a PR on. If in doubt, pick MEDIUM.

References

references is required when the rule depends on:

• A specific Python version
• Stdlib behavior with versioned semantics
• Third-party library behavior (Pydantic, mypy, ruff)
• A PEP

Link to primary sources. Blog posts drift; PEPs don't.

Keep Old Names as Deprecated Aliases

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.

Incorrect (rename breaks existing code immediately):

# v1.0
def get_user(user_id: str) -> User: ...

# v1.1
def fetch_user(user_id: str) -> User: ...  # v1.0 callers now crash

Correct (Python 3.13+: `@warnings.deprecated` marks the symbol for type checkers too):

import warnings

@warnings.deprecated("get_user is deprecated; use fetch_user instead.")
def get_user(user_id: str) -> User:
    return fetch_user(user_id)

On older Pythons, call warnings.warn("...", DeprecationWarning, stacklevel=2) inside the alias body — same runtime effect, minus the static-checker signal.

For renamed parameters, @warnings.deprecated doesn't apply — it decorates whole symbols. Accept the old keyword as a compatibility path with a sentinel default, forward to the new name, and emit warnings.warn(..., DeprecationWarning, stacklevel=2) when the old path is taken. Remove the alias in a later major version. Skip deprecation only when the name was never public (starts with _, not in __all__, not in docs).

Return New Collections from Transforms

A function called filter_active(users) that mutates users in place is a trap — 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).

Incorrect (transform that secretly mutates):

def filter_active(users: list[User]) -> list[User]:
    users[:] = [u for u in users if u.is_active]  # mutates input!
    return users

A caller doing active = filter_active(all_users); log_total(len(all_users)) gets a confusing bug — all_users was modified, but the call site doesn't reveal that.

Correct (return a new list):

def filter_active(users: list[User]) -> list[User]:
    return [u for u in users if u.is_active]

Input is untouched. Behavior matches the name.

When in-place mutation is appropriate: when it's performance-critical on a measured hot path, and the name signals it unambiguously.

def sort_in_place(items: list[int]) -> None:
    items.sort()

def update_status_inplace(user: User, status: str) -> None:
    user.status = status

Name conventions:

• *_in_place / *_inplace — mutates, returns None
• update_* — mutates (if state-management convention) or returns new (if data-transform convention); be consistent within the codebase
• with_*, filter_*, map_*, derive_* — returns new, input untouched

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.

Choose the Simplest Namespace That Matches Ownership and Polymorphism

Python lets the same logic live as a module function, instance method, @classmethod, @staticmethod, or Protocol. None 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?). Start at module scope; promote to a method only when ownership or polymorphism actually demand it.

Incorrect (instance method that doesn't need `self` and isn't overridden):

class DateFormatter:
    def format_iso(self, d: date) -> str:
        return d.isoformat()  # self unused, no subclasses overriding

Correct (module-level function — simpler namespace):

def format_iso(d: date) -> str:
    return d.isoformat()

Conversely, a module function that threads state through a single parameter usually belongs on that type:

# Awkward: mutates `user`, names it in every parameter list, no second caller type
def update_user_preferences(user: User, key: str, value: object) -> None:
    user.prefs[key] = value
    user.last_modified = now()

# Better: the method form matches ownership
class User:
    def update_preference(self, key: str, value: object) -> None:
        self.prefs[key] = value
        self.last_modified = now()

Use @classmethod for alternative constructors (Event.from_json(raw)); the method needs the class for subclass-friendly construction but not an instance. Use a Protocol when several unrelated types need to provide the same interface without a shared base. @staticmethod is the rarest tier — if there's no self and no cls, a module function is usually cleaner. Starting too coupled (everything on a class) is harder to undo than starting too loose (a free function you later move).

Use Keyword-Only Parameters for Optional Config

Positional parameters lock in their order forever — 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. Positional is Python's default; keyword-only is an explicit choice.

Incorrect (positional config — order is now part of the API):

def fetch(url, timeout=30, retries=3, verify_ssl=True, backoff=1.5):
    ...

fetch("https://api.example.com", 60, 5, False)

What do 60, 5, False mean at this call site? Only the function signature knows. And if you want to add user_agent between retries and verify_ssl, every positional call site breaks.

Correct (keyword-only for config params):

def fetch(url, *, timeout=30, retries=3, verify_ssl=True, backoff=1.5):
    ...

fetch("https://api.example.com", timeout=60, retries=5, verify_ssl=False)

The * forces everything after it to be passed by name. Call sites self-document. New params can slot anywhere without breaking callers.

For dataclasses, use `KW_ONLY`:

from dataclasses import dataclass, KW_ONLY

@dataclass
class FetchOptions:
    url: str
    _: KW_ONLY
    timeout: int = 30
    retries: int = 3
    verify_ssl: bool = True
    backoff: float = 1.5

Callers must pass timeout=, retries=, etc. by name.

Heuristic: the first one or two params can be positional (the "thing" the function operates on). Everything else — especially optional configuration — should be keyword-only.

For public APIs this is non-negotiable: once a library ships positional config params, every reorder or addition is a breaking change.

Keep Data Models Flat and Non-Redundant

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.

Incorrect (redundant fields, single-key wrappers, unnecessary lists):

from dataclasses import dataclass

@dataclass
class ToolReturn:
    tool_name: str                           # also in parent
    call_id: str                             # also in parent
    content: dict[str, object]               # single-key wrapper around return_value
    return_value: dict[str, object]          # duplicated in content
    messages: list[Message]                  # always contains exactly one Message

@dataclass
class ToolCall:
    tool_name: str
    call_id: str
    return_part: ToolReturn

tool_name and call_id are carried on both the parent and the child — they'll drift. content wraps return_value. messages is a list that always has length one.

Correct (flat, non-redundant):

from dataclasses import dataclass

@dataclass
class ToolReturn:
    content: object  # the actual return value, unwrapped
    message: Message

@dataclass
class ToolCall:
    tool_name: str
    call_id: str
    return_part: ToolReturn

tool_name and call_id live on the parent only. content holds the value directly. message is singular because there's only ever one.

Check for:

• Fields that exist on both parent and child (pick one, usually parent)
• data: {"value": X} single-key wrappers (unwrap to data: X)
• Lists that always contain exactly one element (use a scalar)
• Fields that are computed from other fields (derive, don't store)

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.

Related: data-derive-dont-store is the same idea at the field level — if one field is computable from another, compute it, don't store it.

Avoid Boolean Flag Parameters in Public APIs

A boolean parameter is a binary mode switch hiding behind a generic type. download(url, True, False, True) is unreadable, the body branches on the flag with near-duplicate paths, and adding a third mode later breaks the API. 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):

def export_report(rows: list[Row], to_csv: bool = True, compress: bool = False) -> bytes:
    data = render_csv(rows) if to_csv else render_json(rows)
    return gzip.compress(data) if compress else data

export_report(rows, False, True)   # JSON, compressed? CSV, compressed? Reader can't tell.

Correct (`Literal` parameter when the modes share most of the body):

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; call sites read meaningfully. Use an Enum when modes carry behavior or constants (e.g. CompressionLevel.BEST with value 9). Split into separate functions when bodies barely overlap and composition is orthogonal.

A single keyword-only bool is still fine when the name clearly answers "what does True mean?" (include_archived=True, strict=True, dry_run=True) and the body is a small filter rather than two near-duplicate paths. Read call sites out loud: export_report(rows, True, False) fails; export_report(rows, format="csv", compress=True) passes.

Don't Access Private Attributes

_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):

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):

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.

Order Required Fields Before Optional Fields

Python's dataclass implementation requires fields without defaults to precede fields with defaults — 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.

Incorrect (raises `TypeError`):

from dataclasses import dataclass

@dataclass
class Tool:
    name: str
    description: str = ""
    version: str         # TypeError: non-default argument follows default argument

Correct (required fields first):

from dataclasses import dataclass

@dataclass
class Tool:
    name: str
    version: str
    description: str = ""

When a required field must come after an optional one: use keyword-only with KW_ONLY. This lets you reorder freely while still enforcing "required" via the type system:

from dataclasses import dataclass, KW_ONLY

@dataclass
class Tool:
    name: str
    _: KW_ONLY
    description: str = ""
    version: str  # required, keyword-only — order no longer constrained

Everything after _: KW_ONLY is keyword-only, so the "required before optional" rule stops applying — the caller must pass them by name.

Same rule applies to function parameters:

# bad: positional default before positional required
def connect(host="localhost", port): ...  # SyntaxError

# good: required first
def connect(port, host="localhost"): ...

# also good: keyword-only lets you mix freely
def connect(*, port, host="localhost", retries): ...

Underscore Prefix for Private Names

Names that start with _ are internal. Names that don't are public — and public means "backward-compatible forever unless deprecated." Without language-level enforcement, implementation details often stay public; underscore them on the way in, not after they've leaked.

Incorrect (implementation detail treated as public):

# mymodule.py
def format_date(d):
    return _to_iso_string(d)

def to_iso_string(d):    # helper — but no underscore, so it's public
    return d.isoformat()

__all__ = ["format_date", "to_iso_string"]  # accidentally exported

Now to_iso_string is part of the module's public API. Changing its signature, renaming it, or removing it breaks anyone who imported it.

Correct (underscore the helper; exclude from `__all__`):

# mymodule.py
def format_date(d):
    return _to_iso_string(d)

def _to_iso_string(d):
    return d.isoformat()

__all__ = ["format_date"]

_to_iso_string is clearly internal. You can rename it, delete it, change its signature — no backward-compat obligation.

Same rule for class attributes and methods:

class Cache:
    def get(self, key: str) -> object | None: ...     # public
    def _evict_lru(self) -> None: ...                  # internal helper
    def _entries(self) -> dict[str, object]: ...       # internal state access

Don't reach into `_private` from outside. If you find yourself writing obj._internal, either (a) the attribute should be public and the owner should know, or (b) the design has a gap — add a public method instead. Reaching into _private couples you to implementation details that may change.

`__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.

Use Timezone-Aware Datetimes at Boundaries

A datetime with no tzinfo is naive: two naive datetimes that look identical may refer to different absolute moments. Naive values leak into databases, JSON, logs, and inter-service messages, then surface as off-by-hours bugs during DST or when hosts differ. At any boundary the value crosses (HTTP, DB, queue, file, log, comparison), it must be timezone-aware. Store and transport in UTC; convert to local zones at display.

Incorrect (`datetime.utcnow()` returns naive; deprecated in 3.12+):

from datetime import datetime

def stamp() -> datetime:
    return datetime.utcnow()              # naive — a serializer reading it as local time writes the wrong value

Correct (UTC-aware at the boundary; local zone only for display):

from datetime import datetime, timezone
from zoneinfo import ZoneInfo

def stamp() -> datetime:
    return datetime.now(timezone.utc)     # aware; round-trips through isoformat() cleanly

stored = datetime.now(timezone.utc)
display = stored.astimezone(ZoneInfo("America/Los_Angeles"))  # named zone, DST handled

zoneinfo (3.9+, PEP 615) reads from system tzdata and handles DST and historical offsets. Use named zones ("America/Los_Angeles"), not raw offsets (-08:00).

Parsing input: if callers can send naive datetimes, decide once whether to reject or assume a fixed zone. Never silently treat naive as UTC. For Pydantic v2, AwareDatetime rejects naive values at the model boundary. For PostgreSQL, use TIMESTAMPTZ; for SQLite/MySQL, store ISO-8601 strings with +00:00 or epoch milliseconds.

Naive is acceptable only inside a tight block where every value is naive and the timezone is documented in scope, or for pure date arithmetic (use date, not datetime). If the value outlives the function it's created in, it should be aware.

Delete Dead Variants

If a type has a variant that is never constructed — a status: Literal["open", "closed", "archived"] where "archived" is never set — 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.

Incorrect ("archived" variant is declared but never produced):

from typing import Literal

OrderStatus = Literal["open", "paid", "shipped", "archived"]

def render_status(status: OrderStatus) -> str:
    match status:
        case "open": return "Awaiting payment"
        case "paid": return "Preparing to ship"
        case "shipped": return "In transit"
        case "archived": return "Archived"  # when does this branch ever run?

Grep the codebase: nothing assigns "archived". That branch is unreachable, yet every consumer must handle it. It's a ghost.

Correct (delete it):

from typing import Literal

OrderStatus = Literal["open", "paid", "shipped"]

def render_status(status: OrderStatus) -> str:
    match status:
        case "open": return "Awaiting payment"
        case "paid": return "Preparing to ship"
        case "shipped": return "In transit"

One fewer imaginary case. When "archived" actually becomes a requirement, add it then — tied to the real code that creates it.

When NOT to delete: if the variant exists in serialized data (old database rows, historical JSON) you still need to parse, keep it — but mark the non-canonical variants clearly (e.g., with a comment pointing to the migration that will remove them).

Derive, Don't Store

Every boolean you add doubles the theoretical state space. When a value can be computed from data you already have, do not store it. Cached derived values require multiple mutation sites to stay in sync — and they won't.

Incorrect (four flags that must be kept in sync):

from dataclasses import dataclass

@dataclass
class ThreadState:
    was_interrupted: bool
    did_assistant_finish: bool
    did_assistant_error: bool
    was_tool_call_only: bool

def should_show_footer(state: ThreadState) -> bool:
    return (
        state.did_assistant_finish
        and not state.was_interrupted
        and not state.did_assistant_error
        and not state.was_tool_call_only
    )

Four fields to answer one question. Four mutation sites elsewhere that must keep them synchronized. One missed update and the footer lies.

Correct (derive from the event log):

def should_show_footer(events: list[SessionEvent]) -> bool:
    latest = get_latest_assistant_message(events)
    if latest is None:
        return False
    return (
        latest.completed
        and not latest.error
        and latest.finish_reason != "tool_calls"
    )

The answer is now computed from evidence that already exists. No sync required — one source of truth.

When NOT to derive:

• The domain genuinely has a state machine with ordered transitions (a checkout step is the state, not a cached conclusion)
• Temporal or external data that cannot be re-derived (timestamps from async processes, API responses needed downstream)
• The derivation is meaningfully more expensive than the stored value and you've measured the cost

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.

Use Discriminated Unions Over Optional Bags

Every optional field is a question the rest of the codebase must answer every time it touches the data. Optional fields accumulate as features grow, producing models where half the combinations are semantically invalid. Use a tagged (discriminated) union so the type system enforces which fields travel together.

Incorrect (optional fields create impossible state combinations):

from dataclasses import dataclass
from typing import Literal

@dataclass
class PaymentState:
    status: Literal["idle", "processing", "settled"]
    gateway: Literal["stripe", "paypal"] | None = None
    transaction_id: str | None = None
    initiated_at: str | None = None
    settled_at: str | None = None

When status == "idle", should gateway exist? The type says maybe. When status == "settled", is settled_at guaranteed? The type says no. Every consumer defensively checks for None on fields that must be present, or forgets to check fields that might not be.

Correct (each variant carries exactly the fields it needs):

from dataclasses import dataclass
from typing import Literal

@dataclass
class PaymentIdle:
    status: Literal["idle"] = "idle"

@dataclass
class PaymentProcessing:
    gateway: Literal["stripe", "paypal"]
    transaction_id: str
    initiated_at: str
    status: Literal["processing"] = "processing"

@dataclass
class PaymentSettled:
    gateway: Literal["stripe", "paypal"]
    transaction_id: str
    settled_at: str
    status: Literal["settled"] = "settled"

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 (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.

Related: data-explicit-variants applies the same idea at the behavior level — split a mode-flag class into one class per mode. Use discriminated unions when the variants are data; use explicit variants when the variants have meaningfully different methods.

Encapsulate Mutable State in the Narrowest Clear Scope

Give mutable state the narrowest scope where the surrounding code still reads clearly. A closure fits when the interface is one or two callables and nothing needs to inspect the state. A focused class fits when state has identity — multiple methods share it, or tests, logs, or subclasses need to see it. A wide-open class where every method can touch the state is how invariants rot.

Incorrect (state visible to every method on the class — too wide):

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  # touched by every method

    def queue_send(self, text: str) -> None: ...
    def flush_now(self) -> None: ...
    def something_else(self) -> None: ...  # nothing prevents a bug here

Correct (focused class — state scoped to the methods that need it):

@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

A closure returning (trigger, clear) is the right alternative when no one needs to inspect, type, serialize, or mock the state — the surface is just the two callables. Module-level globals deserve more pushback than either; prefer dependency injection.

Create Explicit Variants Instead of Mode Flags

When a class grows is_thread, is_editing, is_forwarding flags — or a mode parameter like mode: Literal["thread", "edit", "forward"] — stop. Each flag doubles the state space; each mode check adds conditional logic at every call site. Split into explicit classes instead.

Incorrect (one class, many modes, exponential conditionals):

@dataclass
class MessageComposer:
    on_submit: Callable[[str], None]
    mode: Literal["channel", "thread", "dm_thread", "edit", "forward"]
    channel_id: str | None = None
    dm_id: str | None = None
    message_id: str | None = None

    def render(self) -> Frame:
        if self.mode == "dm_thread":
            extra = AlsoSendToDMField(self.dm_id)
        elif self.mode == "thread":
            extra = AlsoSendToChannelField(self.channel_id)
        else:
            extra = None
        if self.mode == "edit":
            actions = EditActions()
        elif self.mode == "forward":
            actions = ForwardActions()
        else:
            actions = DefaultActions()
        return Frame(extra, actions)

MessageComposer(mode="thread", channel_id=x) — which combinations are valid? Readers have to inspect the implementation to know.

Correct (explicit variants; each self-contained):

@dataclass
class ChannelComposer:
    channel_id: str
    on_submit: Callable[[str], None]
    def render(self) -> Frame:
        return Frame(extra=None, actions=DefaultActions())

@dataclass
class ThreadComposer:
    channel_id: str
    on_submit: Callable[[str], None]
    def render(self) -> Frame:
        return Frame(AlsoSendToChannelField(self.channel_id), DefaultActions())

@dataclass
class EditMessageComposer:
    message_id: str
    on_submit: Callable[[str], None]
    def render(self) -> Frame:
        return Frame(extra=None, actions=EditActions())

Each class declares the fields its variant actually needs. Impossible combinations are unrepresentable. When variants genuinely share logic, extract helpers or a small base class that holds the common interface only — not a mega-class that mode-switches internally.

Related: this rule is about splitting behavior across classes. data-discriminated-unions is the same idea at the data-shape level — tag the variants so consumers can narrow with match/if isinstance. Reach for either when optional fields start accumulating to encode modes.

Never Use Mutable Default Arguments

A default argument is evaluated once, when the def statement runs — not each call. A mutable default ([], {}, set(), a dataclass instance) is therefore shared across every call that doesn't override it. Appending to the "default" list on one call mutates the default for every subsequent call. The same trap applies to dataclass and Pydantic field defaults. Always use None + body construction, or default_factory.

Incorrect (the `[]` is one object, reused across calls):

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

Correct (function — sentinel + per-call construction):

def append_item(item: int, items: list[int] | None = None) -> list[int]:
    if items is None:
        items = []
    items.append(item)
    return items

Correct (dataclass / Pydantic — `default_factory` calls the constructor per instance):

from dataclasses import dataclass, field
from pydantic import BaseModel, Field

@dataclass
class User:
    tags: list[str] = field(default_factory=list)

class Config(BaseModel):
    tags: list[str] = Field(default_factory=list)

@dataclass rejects bare mutable defaults with ValueError. Pydantic v2 happens to deep-copy the default for each instance, but Field(default_factory=list) makes the intent explicit and survives version changes. Safe to use directly as defaults: tuples, frozensets, strings, ints, None, and frozen dataclasses — anything that can't be mutated.

Pick a Mutation Contract

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.

Incorrect (mutates and returns — callers can't tell which to use):

def with_pending_action(state: AppState, action: str) -> AppState:
    state.pending_action = action  # mutation
    return state                   # and return

A caller reading new_state = with_pending_action(state, "confirm") reasonably assumes state is unchanged. It isn't. Another caller reads with_pending_action(state, "confirm") (ignoring the return) and assumes that's fine. It is — but only because the mutation happened. Two callers, two wrong mental models.

Correct (mutate, return None):

def apply_pending_action(state: AppState, action: str) -> None:
    state.pending_action = action

The None return and the imperative verb (apply_) signal that this is a command. Caller knows the input was modified.

Also correct (clone, return new):

from dataclasses import replace

def with_pending_action(state: AppState, action: str) -> AppState:
    return replace(state, pending_action=action)

with_ naming signals a functional transform. The input is untouched; the caller must use the returned value.

Naming conventions:

• apply_*, set_*, update_*_inplace — mutate, return None
• with_*, update_*, derive_* — return a new value, leave input alone

The contract should be obvious from the name and signature without reading the body.

Brand Primitive IDs With NewType

When user_id and team_id are both str, a function accepting UserId will happily take a TeamId and fail at runtime — or worse, silently return wrong data. NewType makes them distinct at the type level without runtime overhead.

Incorrect (interchangeable strings):

UserId = str
TeamId = str

def fetch_user(user_id: UserId) -> User: ...

team_id: TeamId = "team_xyz"
fetch_user(team_id)  # type checker is fine with this — runtime crash

UserId = str is a type alias, not a new type. The checker treats them identically.

Correct (NewType creates a distinct nominal type):

from typing import NewType

UserId = NewType("UserId", str)
TeamId = NewType("TeamId", str)

def fetch_user(user_id: UserId) -> User: ...

team_id = TeamId("team_xyz")
fetch_user(team_id)  # type error: TeamId is not UserId

At runtime, UserId("abc") is just the string "abc" — no wrapper, no overhead. At type-check time, the checker refuses to confuse them.

Construct at the boundary: wrap raw strings as soon as they enter the system (API deserialization, DB rows). Once wrapped, they flow through the codebase as the branded type, and the checker enforces correctness.

When NOT to brand: short-lived local variables, truly interchangeable strings (raw log message bodies, arbitrary user text). Reserve branding for domain identifiers that must not be mixed up.

Phase Related Optional Fields Into Nested Structs

When fields are "all present or all absent" in practice, don't model them as eight independent optionals at the top level. The flattened alternative forces consumers to profile.first_name or defaults.first_name eight times, and the type says nothing about which fields co-occur.

Incorrect (twelve independent optionals; co-occurrence invisible to the type):

@dataclass
class UserProfile:
    first_name: str | None = None
    last_name: str | None = None
    email: str | None = None
    phone: str | None = None
    company: str | None = None
    job_title: str | None = None
    billing_address: str | None = None
    billing_city: str | None = None
    billing_zip: str | None = None
    card_last4: str | None = None
    card_brand: str | None = None
    card_expires: str | None = None

Correct (grouped into phases; one optional check per group):

@dataclass
class Identity:
    first_name: str
    last_name: str
    email: str
    phone: str | None = None

@dataclass
class Billing:
    address: str
    city: str
    zip_code: str
    card_last4: str
    card_brand: str
    card_expires: str

@dataclass
class UserProfile:
    identity: Identity | None = None
    billing: Billing | None = None

Consumers check one optional: if profile.billing is not None: use profile.billing.card_last4. Every billing field is guaranteed present when billing is. The type system enforces the co-occurrence that was always true in practice. Rule of thumb: three or more optionals that always set/unset together belong in a nested struct.

Use a Sentinel Object When None Is a Real Domain Value

When None carries semantic meaning — "user cleared this field," "no parent," "no assignee" — None can't also be the "not provided" default. Use a private sentinel instead. The sentinel is a unique object compared with is, never ==. Complements types-remove-redundant-optional: that rule says drop | None when None is impossible; this one says use a sentinel when None is meaningfully different from "not passed."

Incorrect (`None` does double duty as "absent" and "cleared"):

def update_user(user_id: str, nickname: str | None = None) -> User:
    user = db.get(user_id)
    user.nickname = nickname        # was the caller clearing it, or not passing it?
    db.save(user)
    return user

Correct (sentinel default; `None` means "clear"):

from typing import Final

class _Unset:
    def __repr__(self) -> str:
        return "UNSET"

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"

Pydantic's PATCH pattern uses the same idea — Field(default=UNSET) + filtering {k: v for k, v in model_dump().items() if v is not UNSET} gives you "omitted field" vs. "explicit null."

A private _SENTINEL = object() is fine for tiny internal cases — PEP 661 itself uses that idiom. Prefer a named sentinel class (as above) when the sentinel appears in signatures, reprs, logs, or tracebacks, since the __repr__ makes debugging easier. PEP 661 proposes a standard sentinel(...) helper, but it is still Draft and has not shipped in the typing module; do not claim typing.Sentinel exists. And don't reach for sentinels when None already means "absent" — two-state Optional doesn't need them.

Use assert Only for Debug-Only Internal Invariants

assert is a debug-only statement — Python emits no code for it under -O or PYTHONOPTIMIZE. 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 (runtime contract that vanishes under `-O`):

def transfer_funds(account_id: str, amount: int) -> None:
    assert amount > 0, "amount must be positive"  # stripped under -O
    assert account_id, "account_id required"      # stripped under -O
    ...

Correct (real exceptions for contracts that must hold; `assert` only for programmer-error invariants):

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")
    ...

def process_step(step: Step) -> Result:
    # Step is a closed union; hitting 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-only

If the input crosses a trust boundary (user input, external API, deserialized data), always use a real exception — AssertionError is a poor signal at a system boundary even when it does fire. For exhaustiveness checks specifically, typing.assert_never is sharper than assert False (see error-assert-never-exhaustiveness). Rule of thumb: if you can't articulate why losing the check under -O is acceptable, it shouldn't be an assert.

Use assert_never for Exhaustiveness Checks

typing.assert_never() (Python 3.11+) tells static checkers "every variant has been handled here." If the union later 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 — that statement is debug-only and can be stripped under -O.

Incorrect (the checker can't see this branch is exhaustive):

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}")  # adding PausedStep slips past the checker

Correct (`assert_never` — type error if the union grows, runtime error if reached):

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)

When Step becomes InitStep | RunStep | DoneStep | PausedStep, the checker reports that step is PausedStep at the assert_never call. Use it for closed sums: Literal unions, sealed dataclass hierarchies, discriminated unions, enum dispatch. On Python <3.11, import from typing_extensions — semantics are identical and both mypy and pyright recognize either source.

Consolidate try/except Blocks with the Same Handler

When multiple adjacent operations raise the same exception and need the same handling, merge them into one block. Separate blocks duplicate the handler — and if the handling logic ever changes, you now need to update N places.

Incorrect (three blocks, three copies of the same handler):

def load_config(path: Path) -> Config | None:
    try:
        raw = path.read_text()
    except FileNotFoundError:
        logger.warning("config missing: %s", path)
        return None

    try:
        data = json.loads(raw)
    except json.JSONDecodeError:
        logger.warning("config invalid json: %s", path)
        return None

    try:
        return Config(**data)
    except ValidationError:
        logger.warning("config validation failed: %s", path)
        return None

Three copies of "log and return None." Changing the log level, adding a metric, or switching return value means editing three places.

Correct (one block, one handler):

def load_config(path: Path) -> Config | None:
    try:
        raw = path.read_text()
        data = json.loads(raw)
        return Config(**data)
    except (FileNotFoundError, json.JSONDecodeError, ValidationError) as e:
        logger.warning("config load failed: %s (%s)", path, e)
        return None

One block, one handler, one place to change. The caller sees the same behavior; the implementation is simpler.

When to keep blocks separate:

• Different exceptions need different handling (log-and-return vs. retry vs. re-raise)
• Intermediate values matter for the handler (you want the partial result when the second step fails)
• The blocks are far apart in the function (folding them together would nest too much)

Use `contextlib.suppress` for trivial "ignore the error" cases:

from contextlib import suppress

def try_cleanup(path: Path) -> None:
    with suppress(FileNotFoundError):
        path.unlink()

Cleaner than a full try/except for the "best effort, doesn't matter if it fails" pattern.

Use with / async with for Resource Lifetimes

Any object that owns a finite resource — files, sockets, DB connections, locks, temp dirs, HTTP clients, GPU contexts — should be acquired with with (or async with). The protocol guarantees __exit__ runs even when the body raises, so cleanup happens deterministically. Manual close() forgets to fire on exceptions, leaks resources under failure, and is easy to misorder during refactors.

Incorrect (manual close — leaks on exception):

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()

Correct (`with` — close runs on success, exception, or early return):

def write_report(path: Path, rows: list[Row]) -> None:
    with path.open("w") as f:
        for row in rows:
            f.write(format_row(row))

Async clients use `async with`:

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)

For multiple resources acquired together, contextlib.ExitStack closes all of them in reverse order even if one acquisition raises. Write your own with @contextlib.contextmanager (or @asynccontextmanager) when a resource isn't already a context manager — yield the resource inside a try / finally.

If you're writing try / finally to call close(), release(), or disconnect(), you almost certainly want with instead.

Inherit New Exceptions from Existing Base Exceptions

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.

Incorrect (new exception inherits from Exception directly):

class ToolError(Exception): ...
class ToolTimeoutError(ToolError): ...
class ToolValidationError(ToolError): ...

# New failure mode added in v2:
class ToolRateLimitError(Exception):  # doesn't inherit from ToolError
    ...

# Existing caller:
try:
    run_tool(t, args)
except ToolError:  # no longer catches ToolRateLimitError
    retry()

The existing except ToolError: no longer catches the new error. Every caller must be updated — a silent breaking change.

Correct (inherit from the existing base):

class ToolError(Exception): ...
class ToolTimeoutError(ToolError): ...
class ToolValidationError(ToolError): ...
class ToolRateLimitError(ToolError):  # fits the hierarchy
    ...

# Existing caller still works:
try:
    run_tool(t, args)
except ToolError:  # catches ToolRateLimitError too
    retry()

Callers that want to handle rate limits specifically can add except ToolRateLimitError: — but existing broad handlers keep working.

Design the hierarchy deliberately:

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.

Preserve Tracebacks When Logging Exceptions

When you catch an exception to recover or return a fallback, keep the traceback in the log. logger.error(str(e)) records the message but loses the stack that explains where the failure came from — which is usually the part that makes the log useful.

Incorrect (traceback discarded):

try:
    response = client.fetch(user_id)
except TimeoutError as e:
    logger.error("fetch failed: %s", e)
    return None

The log line records "fetch failed: timeout" with no stack — the caller sees a timeout but can't tell which code path produced it.

Correct (`logger.exception` inside the `except` block):

try:
    response = client.fetch(user_id)
except TimeoutError:
    logger.exception("fetch failed for user_id=%r", user_id)
    return None

logger.exception(...) implicitly attaches exc_info from the current exception, so the traceback is included at ERROR level. Outside an except block — for example, when logging a handled exception from a worker queue — pass exc_info=True (or exc_info=e) explicitly:

logger.error("task %r failed", task_id, exc_info=exc)

When not to log:

• You're re-raising (raise or raise NewError(...) from e) at the same boundary. The outer handler logs once; duplicating here produces two stacks for one failure.
• The exception is expected and recovery is routine (e.g., cache miss). A logger.debug or no log at all is often right.

Use raise ... from to Preserve Exception Causality

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" — which is usually right, but the explicit form is clearer and survives __cause__ suppression in some runtimes.

Incorrect (original exception lost or implicit):

def load_config(path: Path) -> Config:
    try:
        raw = path.read_text()
    except FileNotFoundError:
        raise ConfigError(f"config missing: {path}")  # loses the original FileNotFoundError context

The traceback will still show both — Python implicitly sets __context__ — but the intent isn't explicit, and __cause__ is None, which some tools use to distinguish "we meant this chain" vs. "an error happened while handling."

Correct (explicit `raise ... from`):

def load_config(path: Path) -> Config:
    try:
        raw = path.read_text()
    except FileNotFoundError as e:
        raise ConfigError(f"config missing: {path}") from e

The traceback prints "The above exception was the direct cause of the following exception" — a deliberate chain. __cause__ is set, so programmatic handlers and logging can walk the chain cleanly.

Use `from None` to suppress the context:

When the original exception is genuinely internal and the caller shouldn't see it:

def parse_timestamp(s: str) -> datetime:
    try:
        return datetime.fromisoformat(s)
    except ValueError:
        raise ValueError(f"invalid timestamp: {s!r}") from None

The user-facing error is clean (ValueError: invalid timestamp: 'abc') without the implementation's internal ValueError: Invalid isoformat string:.

Three patterns:

• raise NewError() from original — explicit chain; __cause__ set
• raise NewError() inside except — implicit chain; __context__ set
• raise NewError() from None — suppress the original context entirely

Default to from original when translating between exception types. Reach for from None when the internal cause is noise to the caller.

Use !r Format for Identifiers in Error Messages

{name!r} calls repr(name) — 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.

Incorrect (ambiguous formatting):

raise ValueError(f"Tool {tool_name} not found in registry")
# "Tool  not found in registry" — did tool_name have leading/trailing spaces? was it empty?
# "Tool None not found in registry" — was the literal string "None" or actual None?

Correct (`!r` delimits and disambiguates):

raise ValueError(f"Tool {tool_name!r} not found in registry")
# "Tool '' not found in registry"      — clearly empty string
# "Tool 'my tool' not found in registry" — spaces visible
# "Tool None not found in registry"    — unambiguously the None sentinel

Quotes frame the value. None, numbers, and special types render with their repr — always unambiguous.

Apply consistently:

• Names, IDs, paths: !r
• Numeric counts: plain {} (e.g., f"retrying {count} times")
• Prose: plain {}

raise ValueError(f"tool {name!r} failed after {retries} retries")
raise FileNotFoundError(f"config not found at {path!r}")
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.

Catch Specific Exception Types

Catch the exception types you intend to handle. A broad except Exception: catches every regular error including your own bugs. A bare except: or except BaseException: is worse — it also catches KeyboardInterrupt, SystemExit, and asyncio.CancelledError, which must propagate.

Incorrect (catches your own bugs):

def fetch_user(user_id: str) -> User | None:
    try:
        response = http.get(f"/users/{user_id}")
        return parse_user(response.json())
    except Exception:  # swallows KeyError from a typo in parse_user
        return None

Correct (catch what you actually handle):

def fetch_user(user_id: str) -> User | None:
    try:
        response = http.get(f"/users/{user_id}")
    except (HTTPError, TimeoutError):
        return None
    return parse_user(response.json())  # bugs here propagate

Never use bare except: or except BaseException: — both catch KeyboardInterrupt, SystemExit, and asyncio.CancelledError. A broad except Exception: is fine at an outer boundary when you log and re-raise:

def handle_request(req: Request) -> Response:
    try:
        return process(req)
    except Exception:
        logger.exception("unhandled error in request handler")
        raise

Cancellation semantics (asyncio / anyio): On Python 3.8+, asyncio.CancelledError inherits from BaseException, not Exception. So except Exception: is cancellation-safe — do not flag it as "swallowing cancellation." Only except BaseException: (or bare except:) catches cancellation. If you do catch BaseException or anyio.get_cancelled_exc_class(), re-raise. Wrap must-complete cleanup in asyncio.shield() — under cancellation, finally: blocks race against the cancellation itself.

For meaningful handling, create domain-specific exception types (ToolTimeoutError(ToolExecutionError), etc.) so handlers match on failure mode rather than error text.

Trust Validated State Within the Same Trust Domain

Once a value has been validated and the validated object is immutable, locally constructed, and stays in the same trust domain, internal helpers can skip re-checking it. The cousin of types-trust-the-checker: same principle, but state requires more care because state can change after validation.

Incorrect (re-checking validated immutable state in the same module):

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 not order.items:           # validator guarantees this
        raise ValueError("order must have items")
    if order.total < 0:           # validator guarantees this
        raise ValueError("total must be non-negative")
    for item in order.items:
        process(item)

Correct (trust the invariant):

def fulfill_order(order: ValidatedOrder) -> None:
    for item in order.items:
        process(item)

Keep the check when any of these fail: (1) object is mutable, (2) constructed outside this process (rehydrated from cache, queue, DB), (3) an untyped or plugin caller could produce a bad instance, (4) the object has been exposed to code that might have mutated it. Rehydration is the most common miss — ValidatedOrder.model_validate_json(...) freshly out of the validator is fine; the same type pulled from a cache with no re-validation is not.

When you do trust the invariant, a single assert order.items, "validator guarantees non-empty" at the entry point documents the reasoning without sprinkling defensive if chains through the body.

Validate Input at System Boundaries

Validate once, at the edge — not repeatedly in every internal function. Sprinkling defensive checks throughout the call chain "in case something got through" bloats internals without catching anything a boundary check didn't. Push validation to the boundary (API handler, CLI entrypoint, deserialization), then trust the validated value.

Incorrect (validation scattered through every internal function):

def process_order(order_id: str) -> None:
    if not order_id:
        raise ValueError("order_id required")
    order = load_order(order_id)
    fulfill(order)

def load_order(order_id: str) -> Order:
    if not order_id:  # checked again
        raise ValueError("order_id required")
    ...

def fulfill(order: Order) -> None:
    if not order.id:  # and again
        raise ValueError("order has no id")
    ...

Every internal function re-validates. If the validation rule changes (e.g., order IDs must match a pattern), every copy must change.

Correct (validate at entry; trust internally):

# boundary: the API handler
def handle_fulfill_request(req: Request) -> Response:
    try:
        body = FulfillRequest.model_validate(req.json())  # Pydantic does the work
    except ValidationError as e:
        return error_response(400, str(e))

    process_order(body.order_id)
    return success_response()

# internal: takes a validated value, trusts it
def process_order(order_id: OrderId) -> None:
    order = load_order(order_id)
    fulfill(order)

One validation point. Internal code takes OrderId (a branded NewType) and trusts it — the validation already happened.

Boundaries that need validation:

• HTTP request parsing (headers, path params, query strings, body)
• CLI argument parsing
• Reading files or database rows that originated outside the system
• Message queue consumers
• Foreign API responses

Heuristic: data at a boundary is untrusted. Validate it into a typed model (Pydantic, dataclass with a validator, NewType + explicit check). Once validated, the typed model flows through internal code unchecked.

Fail fast: validate before expensive operations. Don't read a 10MB file, parse it, and then reject it for missing a required field — check the field first.

No Duplicate Imports

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.

Incorrect (same name imported twice):

import json
from typing import Any
from pathlib import Path

# ... later in the file, after a later edit ...
from pathlib import Path       # duplicate
from pathlib import Path as P  # different alias, same underlying import

The first duplicate is pure redundancy. The second is worse — now Path and P both exist in the namespace, pointing to the same class.

Correct (one import per name):

import json
from typing import Any
from pathlib import Path

If two aliases are genuinely needed (very rare — usually a code-smell), pick one:

from pathlib import Path  # use this name everywhere

When "duplicates" are actually distinct:

from foo import bar
from foo.baz import bar as baz_bar  # different bar, aliased to avoid collision

These are different objects with the same name in different namespaces. Aliasing one disambiguates. This is fine — but it's not a duplicate; the names differ.

Detection:

• ruff check --select F811 flags redefinitions
• pyflakes also catches these

Add to pre-commit or CI.

Root cause: duplicate imports usually appear after:

• Merging branches that both added the same import
• An IDE auto-import on top of an existing import
• Refactoring that copied a block without cleaning up the imports

Reviewing the imports block after any merge or mass edit catches these before they land.

Keep Modules Cheap to Import

Anything at module top-level — opening files, reading env vars, building large data structures, connecting to databases, registering handlers, hitting the network — runs every time anything in that module is imported. That cost compounds across CLI cold-starts, test collection, worker pools, and serverless functions. It also makes modules hard to mock. Push side effects into functions, factories, or lazy properties that callers invoke explicitly.

Incorrect (network call, heavy init, and env read at import):

# config.py
import requests

CONFIG = requests.get("https://config.example.com/v1").json()  # network at import
DB_URL = CONFIG["db_url"]

# embeddings.py
MODEL = SentenceTransformer("all-MiniLM-L6-v2")                # 90 MB download + GPU init

# keys.py
API_KEY = os.environ["MY_API_KEY"]                             # KeyError on import if unset

Importing any of these for a type or constant triggers the work. A CLI's --help takes seconds; offline CI breaks; reading the module docstring fails without the env var set.

Correct (lazy — pay only when the feature runs):

from functools import cache

@cache
def get_config() -> dict[str, object]:
    return requests.get("https://config.example.com/v1").json()

@cache
def get_model() -> "SentenceTransformer":
    from sentence_transformers import SentenceTransformer
    return SentenceTransformer("all-MiniLM-L6-v2")

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

@cache gives you "once per process" semantics without the "every import" cost.

Fine at import time: pure-Python constants, re.compile for a static pattern, class and function definitions, stdlib imports, cheap registrations. Push out of import time: network/disk I/O, subprocess launches, large model loads, env-var reads that may fail, DB/queue connections, heavy third-party imports the module doesn't unconditionally use. If python -c "import yourpackage" takes more than ~100 ms or hits the network, something at module scope should be deferred.

Handle Optional Dependencies Explicitly

When a package has optional integrations, importing the module should not require every optional dep. Handle ImportError at module scope with a message pointing to the install extra; raising None-valued placeholders produces AttributeError far from the root cause.

Incorrect (silently swallowing the ImportError):

try:
    import anthropic
except ImportError:
    anthropic = None  # downstream code crashes with AttributeError later

class AnthropicProvider:
    def __init__(self):
        client = anthropic.Client()  # AttributeError: 'NoneType' has no 'Client'

Correct (raise with an actionable install hint; preserve the original cause):

try:
    import anthropic
except ImportError as e:
    raise ImportError(
        "anthropic is required for AnthropicProvider. "
        "Install with: pip install 'mylib[anthropic]'"
    ) from e

class AnthropicProvider:
    ...

If the dep is optional at the feature level rather than the module level, defer the import into the function that needs it — users who never call it never pay the cost. Pair module-scope optional imports with a TYPE_CHECKING block (see types-type-checking-imports) when type hints should resolve without requiring the runtime dep.

Remove Unused Imports

Every import is a declaration of "this module depends on X." Unused imports lie about dependencies, add reading noise, risk circular imports, and mask refactoring errors — the import survives long after the only call site was deleted.

Incorrect (imports for names that aren't used):

import json
import re
from typing import Any, Optional, Union

from .helpers import validate, format_date  # format_date never used

def compact(data: dict[str, Any]) -> str:
    return json.dumps(data, separators=(",", ":"))

Correct (just what's needed):

import json
from typing import Any

def compact(data: dict[str, Any]) -> str:
    return json.dumps(data, separators=(",", ":"))

ruff check --select F401 flags unused imports — wire it into pre-commit or CI. If a module intentionally re-exports names (common in __init__.py), use the from .client import Client as Client form or list them in __all__; both signal "intentional, not forgotten." If an import is used only in annotations, move it under if TYPE_CHECKING: (see types-type-checking-imports). Keep an otherwise-unused import only when importing it has a required side effect (plugin self-registration) — and comment it: # noqa: F401 — registers handlers at import time.

Scope Helpers and Constants to Their Usage Site

Scope tiny helpers and one-off constants near their only use. Promote a helper to module scope when it is substantial, reused, independently testable, expensive to rebuild, or part of the module's contract. Nested functions are cheap inside a small callable, but stacking nontrivial logic inside another function hurts readability and makes the helper harder to test directly.

Incorrect (tiny one-off constant hoisted into the module namespace):

# somewhere in a 500-line module
_DEFAULT_MAX_LENGTH = 280

def summarize(text: str) -> str:
    normalized = " ".join(text.split())
    return normalized[:_DEFAULT_MAX_LENGTH]

# ... 400 more lines, no other use of _DEFAULT_MAX_LENGTH

A future reader sees _DEFAULT_MAX_LENGTH at module scope and assumes it's shared. If it isn't, that's noise.

Correct (trivial constant local to its one caller):

def summarize(text: str) -> str:
    DEFAULT_MAX_LENGTH = 280
    normalized = " ".join(text.split())
    return normalized[:DEFAULT_MAX_LENGTH]

When to promote to module scope:

• Reused by more than one function
• Substantial enough to want its own unit tests
• Expensive to rebuild per call (compiled regex, TypeAdapter, precomputed table)
• Part of the module's contract (constants referenced by name, e.g., MyClass.DEFAULT_TIMEOUT)
• Needs to be patched in tests (module-level constants are easy to monkeypatch)

Prefer module-level over a nested def whenever the helper has real logic. Nested functions are appropriate for short closures or very small transforms; they stop being appropriate once the helper grows past a few lines.

Imports follow the opposite default (see imports-top-of-file): imports live at the top of the module unless there's a specific reason (optional deps, circular, expensive-to-import) to defer them.

Place Imports at the Top of the File

Imports belong at the top of the module, grouped (stdlib, third-party, local) with blank lines between groups. Inline imports inside functions hide dependencies from readers, confuse static analysis, and surprise anyone debugging a ModuleNotFoundError raised in the middle of a call. ruff / isort automate the grouping.

Incorrect (imports scattered through function bodies):

def fetch_user(user_id: str) -> User:
    import requests  # hidden dependency
    response = requests.get(f"/users/{user_id}")
    return User(**response.json())

def process():
    from .helpers import validate  # easily missed
    import json                    # another one
    data = json.dumps(result)

Correct (all imports at the top, PEP 8 ordering):

import json
from typing import Any

import requests

from .helpers import validate


def fetch_user(user_id: str) -> User:
    response = requests.get(f"/users/{user_id}")
    return User(**response.json())

Inline imports are legitimate only for: breaking circular imports (add a comment so readers don't "fix" it), deferring truly optional/heavy deps behind a runtime gate (see imports-optional-dependencies), or avoiding module-load-time side effects. Outside those cases, top-of-file is the rule.

Use Consistent Terminology Across Code and Docs

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 — in code, docstrings, error messages, and external docs.

Incorrect (same concept, three names):

# module_a.py
def get_last_message(session): ...

# module_b.py
def fetch_latest(session): ...

# module_c.py
def current_message(session): ...

# error message
raise ValueError("no recent message found")

A user searching for "latest message" in code finds one match; in docs, another; in error messages, a third. The concept is fragmented.

Correct (one term, everywhere):

# everywhere
def get_latest_message(session): ...
raise ValueError("no latest message found")
# docs: "The latest message is..."

One word per concept. Search works.

Choose deliberately — and write it down:

• latest vs. last vs. most_recent — pick one
• message vs. msg — pick one
• tool vs. function vs. capability — pick one
• user vs. account vs. member — pick one

If the codebase has a GLOSSARY.md or CONTRIBUTING.md, list the canonical terms and their boundaries. If not, pick through current usage by grepping — whichever is most common wins.

When different terms are genuinely different things:

Sometimes "message" and "msg" mean different things (a full message object vs. a short string body). That's fine — but then the distinction should be explicit and documented. If you need two terms, you need two concepts.

Refactoring legacy inconsistency:

• Add the canonical alias first, deprecate the old
• Update docstrings and error messages in the same PR
• Don't let PRs introduce new variants (message, msg, messageObj in one diff) — pick one, stick to it

Why it matters: users grep. Docs search. Error messages end up in Stack Overflow questions. When terminology fragments, every question becomes "how do I look this up?" — and the answer gets split across three terms that mean the same thing.

Drop Redundant Prefixes When Context Is Clear

When a field is accessed as tool_config.tool_description, the tool_ prefix adds nothing — the class name already provides that context. Repeating the class name in every field ("just to be clear") produces noise that makes real information harder to find.

Incorrect (prefix repeats the class context):

from dataclasses import dataclass

@dataclass
class ToolConfig:
    tool_name: str
    tool_description: str
    tool_version: str
    tool_timeout: float

Every access reads config.tool_name, config.tool_description. The tool_ adds zero information.

Correct (name without the redundant prefix):

@dataclass
class ToolConfig:
    name: str
    description: str
    version: str
    timeout: float

Now config.name, config.description — shorter and just as clear.

The rule: drop the prefix when the class, module, or variable name already establishes the context.

Examples from real codebases:

# before → after
server.server_label       → server.label
mcp_server.mcp_version    → mcp_server.version   (or just version on the class)
user_profile.user_id      → user_profile.user_id (probably keep — "id" alone is too generic)

The last example is a judgment call. user_profile.id would be unambiguous in context, but user_id reads well when passing it around as a variable. Lean toward dropping when it's a field on a class, keep the prefix when the value travels as a parameter.

When to keep a prefix:

• The field is a foreign key to another entity (user_id on a Post, author_id on a Comment) — the prefix signals what it points to
• Two related fields share a type and need disambiguation (created_at vs. updated_at)
• Dropping the prefix makes the name ambiguous (format could mean many things; date_format is specific)

Be consistent: whatever you pick, apply it uniformly across sibling fields. tool_name with description (mixed) reads worse than either all-prefixed or all-bare.

Avoid Redundant Type Suffixes in Names

user_list: list[User], config_dict: dict[str, str], name_str: str — the suffix repeats what the type annotation already says. Python has type annotations; let them do the work. Hungarian-style suffixes "make the type clear" at the cost of restating what's already on the next token.

Incorrect (suffix restates the type):

def filter_users(user_list: list[User], active_dict: dict[str, bool]) -> list[User]:
    name_str = user_list[0].name_str
    result_list: list[User] = []
    ...

Every name repeats its type. The code is harder to read because the meaningful word is buried.

Correct (let types speak):

def filter_users(users: list[User], active_by_id: dict[str, bool]) -> list[User]:
    name = users[0].name
    result: list[User] = []
    ...

users and active_by_id describe what the value is for; the types describe the shape.

Suffixes to drop:

• _list, _dict, _set, _tuple — shape is in the type
• _str, _int, _float, _bool — primitive type is in the type
• Value, Type, Class — usually redundant (UserType vs. just User)

When a type-ish suffix genuinely helps:

• _by_key names signal the dict's key (users_by_id, posts_by_author)
• _count, _index, _id signal the semantic role, not the type
• _bytes / _str on a variable that could be either (body_bytes vs. body_text) — disambiguating two valid forms is useful

Class names: don't suffix with Class. UserClass is just User. The definition is class User:.

Enum values: keep them short and meaningful. Color.RED reads better than Color.COLOR_RED.

Private helpers: same rule applies. _parse_user_dict where the return is dict[str, User] — just _parse_users.

Exception classes: convention is to end with Error (ValidationError, TimeoutError). This is the established Python pattern and worth keeping.

Rename When Behavior Changes

A function's name is a promise about what it does. When the behavior changes — wider scope, different return type, side effects added — the old name lies. Names often stay stable because "it's a smaller diff"; the cost is that every future reader has to figure out the name is wrong.

Incorrect (name no longer matches behavior):

# v1: called only for function tools
def _call_function_tool(tool: FunctionTool, args: dict) -> Result:
    return tool.invoke(args)

# v2: extended to handle output tools too, but name unchanged
def _call_function_tool(tool: FunctionTool | OutputTool, args: dict) -> Result:
    if isinstance(tool, FunctionTool):
        return tool.invoke(args)
    return tool.build_output(args)  # wait, this isn't a "function tool"

Every reader now has to re-learn what _call_function_tool means. The name says "function tool"; the body says "any tool."

Correct (rename to reflect the wider scope):

def _call_tool(tool: FunctionTool | OutputTool, args: dict) -> Result:
    if isinstance(tool, FunctionTool):
        return tool.invoke(args)
    return tool.build_output(args)

Name matches behavior again.

Signals that a rename is due:

• The function's scope expanded (handles more types, more cases)
• The return type changed substantively
• Side effects were added or removed
• The function's "level" changed (was a leaf, now orchestrates; was a command, now a query)

When in-place rename is fine:

• Private (_-prefixed) functions — callers are all internal, update them
• Internal helpers with a small number of call sites

When rename needs a migration:

• Public API — add the new name, keep the old as a deprecated alias (see api-deprecated-aliases)
• Widely-used internal helpers — IDE-assisted rename is safer than hand-edit

For method renames across a class hierarchy — use the @override decorator when the intent is to override, and let the checker catch stragglers:

from typing import override

class MemoryToolset(Toolset):
    @override
    def list_tools(self) -> list[Tool]: ...

If Toolset renames list_tools, @override makes the subclass fail type-checking until updated.

Use Specific Parameter and Variable Names

Generic names like id, name, data, info communicate nothing about the value's role. When multiple IDs or data objects share a scope, they collide. Names that convey the semantic role make call sites self-documenting.

Incorrect (generic names — call site is ambiguous):

def transfer(id: str, id2: str, data: dict, info: dict) -> None:
    ...

transfer("u123", "t456", {...}, {...})  # which is sender, which is recipient?

Correct (specific — names carry the semantic role):

def transfer(
    sender_id: str,
    recipient_id: str,
    transfer_data: TransferRequest,
    audit_info: AuditContext,
) -> None:
    ...

Generic is acceptable for truly generic helpers (def first(items: list[T]) -> T), when there's only one of the type in scope (def render(user: User)), or following convention (self, cls, _, loop indices i / j in math contexts). The red flag is ending up with id, id2, id3 or data, info, details, meta all in the same scope — the number suffixes tell you the names aren't doing their job. In nested loops, for user in users beats for x in users once the body is more than a couple of lines.

Use UPPER_CASE for Module Constants

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. A reader seeing default_timeout can't tell at a glance whether it's a constant or a mutable config someone might reassign.

Incorrect (looks like a reassignable variable):

default_timeout = 30
max_retries = 3
allowed_hosts = frozenset({"localhost", "127.0.0.1"})

Correct (UPPER_CASE for constants; `_` prefix for internal):

DEFAULT_TIMEOUT = 30
MAX_RETRIES = 3
ALLOWED_HOSTS = frozenset({"localhost", "127.0.0.1"})
_DEFAULT_CACHE_SIZE = 512

The underscore keeps internal constants out of from module import * and signals they're not part of the public API. Enum members and class-level constants follow the same convention (Color.RED, Cache.DEFAULT_SIZE). For machine-enforced immutability, pair with typing.Final:

from typing import Final

DEFAULT_TIMEOUT: Final[int] = 30  # checker flags any reassignment

Keep lower_case for values that look like constants but aren't — derived from os.environ at import, intentionally reassignable feature flags, or test-mutable hooks. The convention is for intentional constants.

Combine Filter and Map Into One Pass

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.

Incorrect (three passes, two intermediate lists):

def prices_for_sale_items(items: list[Item]) -> list[Decimal]:
    sale_items = [i for i in items if i.on_sale]
    discounted = [i for i in sale_items if i.discount > 0]
    prices = [i.price * (1 - i.discount) for i in discounted]
    return prices

Three allocations, three passes.

Correct (one pass, one list):

def prices_for_sale_items(items: list[Item]) -> list[Decimal]:
    return [
        item.price * (1 - item.discount)
        for item in items
        if item.on_sale and item.discount > 0
    ]

One pass, one list. Conditions combined; mapping in the expression.

When chaining is clearer:

If each step has enough logic that inlining makes the comprehension hard to read, keep them separate — readability wins over a small constant-factor performance gain:

# fine — each step has real logic
eligible = [normalize(u) for u in users if u.tenure_months >= 12]
grouped = group_by_team(eligible)
summaries = [compute_summary(team, members) for team, members in grouped.items()]

For reductions, use the built-in that takes a generator:

# don't build a list just to sum it
total = sum([i.price for i in items if i.on_sale])

# better — generator, no intermediate list
total = sum(i.price for i in items if i.on_sale)

Same for min, max, any, all, ''.join(...).

For multi-step transforms, itertools provides streaming building blocks (chain, islice, groupby). Most of the time, one comprehension is enough.

Compile Static Regex Patterns at Module Level

Compile static regexes at module scope when the pattern is reused, named, or sits on a measured hot path. Python's re module caches recent compiled patterns from the module-level calls (re.search, re.match, etc.), so a one-shot call outside a hot path is not paying a real recompilation cost. The win from hoisting is mostly readability and naming — and, on genuinely hot paths, bypassing the cache lookup.

Incorrect (reused pattern buried inline, no name):

import re

def extract_version(text: str) -> str | None:
    match = re.search(r"v(\d+\.\d+\.\d+)", text)
    return match.group(1) if match else None

The pattern is a named concept (VERSION_RE) shared across the module but inlined anonymously. If a second function needs the same regex, it gets copied.

Correct (compiled once at module scope with a descriptive name):

import re

_VERSION_RE = re.compile(r"v(\d+\.\d+\.\d+)")

def extract_version(text: str) -> str | None:
    match = _VERSION_RE.search(text)
    return match.group(1) if match else None

The name documents intent, other call sites reuse the same object, and a tight loop avoids the internal cache lookup.

Naming:

• _UPPER_CASE for module-level private regex constants (or whatever your project's constant convention is)
• Descriptive names — _VERSION_RE, _EMAIL_RE, not _PATTERN1

Don't hoist when:

• The pattern depends on a runtime value (different per call)
• The regex is one-shot startup parsing and an inline call reads more clearly

Related: the same "build once, use many" instinct applies to other reusable objects — TypeAdapter, json.JSONDecoder with custom hooks, precompiled templates.

Build a Dict Index Instead of Nested Loops

When code says "for each item in A, find the matching item in B," the naive pattern is nested for + if x.id == y.id — that's O(n × m). Build a dict from B once, then it's O(n + m) with each lookup O(1).

Incorrect (nested scan — 100M comparisons for 10k × 10k):

def attach_profiles(users: list[User], profiles: list[Profile]) -> list[EnrichedUser]:
    result = []
    for user in users:
        matching = None
        for profile in profiles:
            if profile.user_id == user.id:
                matching = profile
                break
        result.append(EnrichedUser(user=user, profile=matching))
    return result

Correct (dict index — O(n + m)):

def attach_profiles(users: list[User], profiles: list[Profile]) -> list[EnrichedUser]:
    profiles_by_user = {p.user_id: p for p in profiles}
    return [
        EnrichedUser(user=user, profile=profiles_by_user.get(user.id))
        for user in users
    ]

For one-to-many grouping, collections.defaultdict(list) avoids the "check-then-create" dance: posts_by_author[post.author_id].append(post). itertools.groupby groups already-sorted inputs without building a dict. Nested loops stay fine for small collections (under ~50 × 50), for one-off operations, or when the inner loop has rich logic that doesn't reduce to a key lookup.

Stream with Generators When Memory or First-Result Latency Matters

Generators trade materialization for laziness: one value at a time, no intermediate list, caller can stop early. This is a memory and streaming rule, not "generators are categorically better." When you need every result, re-iterate, want random access, or will sort anyway, a list comprehension is often clearer and sometimes faster.

Incorrect (materializes a multi-GB file three times for a count):

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)

Correct (streaming — constant memory regardless of file size):

def count_errors(path: Path) -> int:
    with path.open() as f:
        return sum(1 for line in f if parse_line(line).level == "ERROR")

Reach for a generator when the input is large, unbounded, or the consumer can stop early (any(), next(), break). Reach for a list when you need len(), iterate more than once, need random access, or will sort the whole sequence. A generator exhausted by the first loop reading zero on the second is a real bug, not a perf issue. itertools (chain, islice, takewhile, groupby) yields lazily for pipelines that stay streaming.

Prefer Tuple Syntax in isinstance() Only on Profiled Hot Paths

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.

This is a micro-optimization, not a correctness rule. Apply it only when:

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.

Incorrect (rewriting `A | B` to `(A, B)` everywhere as a stylistic crusade):

# 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)

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.

Correct (apply only on a measured hot path, with a named module-level tuple):

# 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)

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.

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.

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.

Use functools.lru_cache for Pure Functions

When a function is pure (same input → 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.

Incorrect (recomputing the same answer):

def parse_version(version_str: str) -> Version:
    # called from many call sites, often with the same string
    return Version.parse(version_str)

If 100 call sites ask parse_version("1.2.3"), you parse it 100 times.

Correct (cached):

from functools import lru_cache

@lru_cache(maxsize=256)
def parse_version(version_str: str) -> Version:
    return Version.parse(version_str)

First call parses and stores; subsequent calls return the cached Version. maxsize caps the cache to 256 entries (LRU eviction).

`functools.cache` (Python 3.9+) for unbounded:

from functools import cache

@cache
def load_schema(name: str) -> Schema:
    return Schema.from_file(SCHEMA_DIR / f"{name}.json")

No size limit. Good when the key space is naturally small (like schema names) and entries are expensive to build.

Requirements:

• Arguments must be hashable (no mutable lists, dicts, or sets as args)
• Function must be pure — same inputs produce the same output
• No side effects that callers depend on happening each call

When NOT to cache:

• Arguments are unhashable (convert to tuple first, or use a different strategy)
• The function has meaningful side effects (logging, writes)
• The key space is unbounded and entries are large (cache grows without limit)
• The computation is cheap and the call frequency is low

Hand-rolled caches:

If @lru_cache doesn't fit (unhashable args, multi-level keys, time-based invalidation), build a module-level dict cache — but name it clearly and document the invalidation strategy. Uncontrolled hand-rolled caches leak memory.

For instance methods, prefer `@cached_property` when the "arguments" are just self — see simplify-cached-property.

Use set for Repeated Membership Checks

x in some_list scans the list every time — O(n). x in some_set is a hash lookup — O(1). When you're checking membership repeatedly against the same collection, the set conversion pays for itself quickly.

Incorrect (list membership in a loop):

def filter_allowed(items: list[Item], allowed: list[str]) -> list[Item]:
    return [item for item in items if item.id in allowed]

For each of len(items) checks, in allowed scans the whole list. If both are 10k, that's 100M comparisons.

Correct (convert once, check many):

def filter_allowed(items: list[Item], allowed: list[str]) -> list[Item]:
    allowed_set = set(allowed)
    return [item for item in items if item.id in allowed_set]

Conversion is O(n); each in check is O(1). Total: O(n + m) instead of O(n × m).

When to use `frozenset`:

Module-level constants with fixed membership — can't be modified accidentally, hashable so it can be used as a dict key:

_ADMIN_ROLES: frozenset[str] = frozenset({"admin", "owner", "superuser"})

def is_admin(role: str) -> bool:
    return role in _ADMIN_ROLES

When NOT to convert to set:

• Only checking membership once (conversion costs more than the scan)
• The collection is tiny (under ~10 elements) — list scan is competitive
• Order matters and you need the list semantics

For lookups by key (not just membership), use a dict:

# bad — scanning a list for "the one with this id"
user = next((u for u in users if u.id == target_id), None)

# good — build once, look up many
users_by_id = {u.id: u for u in users}
user = users_by_id.get(target_id)

Same asymptotic improvement as set membership, and you get the associated value instead of just a boolean.

Define TypeAdapter Instances at Module Level

Applicability: Pydantic v2's TypeAdapter. The same principle applies to any object whose constructor does real work.

TypeAdapter builds a validation schema on construction by walking the target type, resolving annotations, and assembling the validation tree. Inside a hot function, every call rebuilds it. Create once at module scope; reuse.

Incorrect (schema rebuilt on every call):

from pydantic import TypeAdapter

def parse_users(raw: bytes) -> list[User]:
    adapter = TypeAdapter(list[User])  # schema built every call
    return adapter.validate_json(raw)

Correct (module-level constant):

_USERS_ADAPTER: TypeAdapter[list[User]] = TypeAdapter(list[User])

def parse_users(raw: bytes) -> list[User]:
    return _USERS_ADAPTER.validate_json(raw)

When the target type depends on a runtime value, cache per type with @functools.cache:

@cache
def _adapter_for(model_type: type) -> TypeAdapter:
    return TypeAdapter(model_type)

The same pattern applies to json.JSONDecoder with custom hooks, msgpack.Packer / Unpacker with configuration, compiled templates, and precomputed lookup tables — anything whose constructor does real work.

Use any() / all() Over Boolean-Flag Loops

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 — more code, more bugs, no short-circuit benefit.

Incorrect (manual flag + break):

def has_admin(users: list[User]) -> bool:
    found = False
    for user in users:
        if user.is_admin:
            found = True
            break
    return found

def all_ready(services: list[Service]) -> bool:
    for s in services:
        if not s.ready:
            return False
    return True

Correct (built-ins):

def has_admin(users: list[User]) -> bool:
    return any(u.is_admin for u in users)

def all_ready(services: list[Service]) -> bool:
    return all(s.ready for s in services)

Both short-circuit — any() stops at the first truthy, all() stops at the first falsy.

Pass a generator, not a list:

# wasteful — builds the full list before checking
any([expensive_check(x) for x in items])

# right — lazy generator, stops at first match
any(expensive_check(x) for x in items)

Other built-ins worth remembering:

# count matches
count = sum(1 for x in items if x.valid)

# min / max with a key
cheapest = min(items, key=lambda x: x.price)

# first matching element (or None)
first_error = next((x for x in items if x.failed), None)

next(generator, default) is the Pythonic "find first or default" — more direct than a loop with an early return.

When to use a loop instead: when you need the loop variable for something else, the logic has side effects, or the condition is too complex to fit in a generator cleanly.

Use @cached_property Only When the Instance Supports It

@cached_property defers expensive derivations until first access and caches the result in instance.__dict__. It fits when the inputs are effectively immutable, the getter is idempotent, the class has a writable __dict__, and nothing is racing on first access. Outside that envelope it masks real bugs: stale caches when inputs mutate, TypeError on __slots__ classes without __dict__, and duplicated work when two threads hit the property simultaneously.

Incorrect (mutable inputs — silent staleness):

from functools import cached_property
from dataclasses import dataclass, field

@dataclass
class Report:
    rows: list[Row] = field(default_factory=list)  # 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

Correct (frozen container; cache cannot go stale):

@dataclass(frozen=True)
class Report:
    rows: tuple[Row, ...]

    @cached_property
    def summary_stats(self) -> Stats:
        return compute_stats(self.rows)

Caveats: not thread-safe — two threads racing on first access can both run the getter. __slots__ classes without "__dict__" raise TypeError at first access. copy.copy carries the cached value over; clear it manually if the copy's inputs differ. 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.

Use Comprehensions Over for+append Loops

Comprehensions express "build a collection from an iterable" in one line. C-style loops with append() have more variables and more places for off-by-one and wrong-list bugs. Reach for a comprehension by default.

Incorrect (imperative loop + append):

def active_usernames(users: list[User]) -> list[str]:
    result = []
    for user in users:
        if user.is_active:
            result.append(user.name)
    return result

Correct (list, dict, set, and generator forms):

def active_usernames(users: list[User]) -> list[str]:
    return [user.name for user in users if user.is_active]

name_to_id = {user.name: user.id for user in users}
unique_tags = {tag for post in posts for tag in post.tags}
total = sum(item.price for item in items)   # generator, no intermediate list

Break a comprehension into a loop when the expression stops reading like English — multi-step logic, side effects, or nested conditionals with intermediate variables are signs the comprehension has outgrown one line. For boolean reductions, prefer any(u.is_admin for u in users) over any([...]) — the generator short-circuits and avoids materializing the list.

Return Early to Flatten Control Flow

When a function has preconditions to check, return as soon as one fails. Deeply nested "if valid, if authorized, if ..." pyramids bury the happy path five levels in. Guard clauses flatten the structure and make the happy path the most visible branch.

Incorrect (pyramid of nesting — the actual work is five levels in):

def process_request(req: Request) -> Response:
    if req.authenticated:
        if req.authorized:
            if req.body is not None:
                if req.body.is_valid:
                    return do_process(req.body)
                else:
                    return error(400, "invalid body")
            else:
                return error(400, "missing body")
        else:
            return error(403, "forbidden")
    else:
        return error(401, "unauthenticated")

Correct (guard clauses; happy path unindented at the end):

def process_request(req: Request) -> Response:
    if not req.authenticated:
        return error(401, "unauthenticated")
    if not req.authorized:
        return error(403, "forbidden")
    if req.body is None:
        return error(400, "missing body")
    if not req.body.is_valid:
        return error(400, "invalid body")

    return do_process(req.body)

The same pattern applies to loops — if not item.active: continue instead of nesting the work inside if item.active:. Keep if/else when the two branches do comparable work ("positive" vs. "negative" vs. "zero"); guard-clause when one branch is an error and the other is the real work.