Python Best Practices

Version 1.3.0 Python Best Practices April 2026

Note: This document is optimized for AI agents and LLMs that maintain, generate, or refactor Python codebases. Humans may also find it useful, but the guidance, examples, and framing prioritize consistency and pattern-matching for AI-assisted workflows.

Abstract

Python software engineering guidelines for agent consumption. 70 rules across 8 categories, prioritized by impact from data modeling and error handling down to naming and import hygiene. Each rule is observational — it describes the pattern and what it costs, shows incorrect and correct code, and cites primary sources where the rule depends on language or library behavior. Rules assume Python 3.11+ as a baseline; rules depending on a higher version (e.g., 3.13 for warnings.deprecated) are called out inline. A rule match is a signal, not a verdict: most rules are design preferences for new code rather than bugs to fix across the repo.

Table of Contents

1. Data Modeling — HIGH
   • 1.1 Brand Primitive IDs With NewType
   • 1.2 Create Explicit Variants Instead of Mode Flags
   • 1.3 Delete Dead Variants
   • 1.4 Derive, Don't Store
   • 1.5 Encapsulate Mutable State in the Narrowest Clear Scope
   • 1.6 Never Use Mutable Default Arguments
   • 1.7 Phase Related Optional Fields Into Nested Structs
   • 1.8 Pick a Mutation Contract
   • 1.9 Use Discriminated Unions Over Optional Bags
   • 1.10 Use Timezone-Aware Datetimes at Boundaries
   • 1.11 Use a Sentinel Object When None Is a Real Domain Value
2. Error Handling — MEDIUM-HIGH
   • 2.1 Catch Specific Exception Types
   • 2.2 Consolidate try/except Blocks with the Same Handler
   • 2.3 Inherit New Exceptions from Existing Base Exceptions
   • 2.4 Preserve Tracebacks When Logging Exceptions
   • 2.5 Trust Validated State Within the Same Trust Domain
   • 2.6 Use !r Format for Identifiers in Error Messages
   • 2.7 Use assert Only for Debug-Only Internal Invariants
   • 2.8 Use assert_never for Exhaustiveness Checks
   • 2.9 Use raise ... from to Preserve Exception Causality
   • 2.10 Use with / async with for Resource Lifetimes
   • 2.11 Validate Input at System Boundaries
3. Type Safety — MEDIUM-HIGH
   • 3.1 Avoid Any Annotations
   • 3.2 Fix Type Definitions Instead of cast()
   • 3.3 Fix Type Errors, Don't Ignore Them
   • 3.4 Narrow Type Signatures to Runtime Reality
   • 3.5 Remove Redundant | None When Values Are Guaranteed
   • 3.6 Trust the Type Checker — Remove Redundant Runtime Checks
   • 3.7 Use Literal Types for Fixed String Sets
   • 3.8 Use TYPE_CHECKING for Optional Dependencies
   • 3.9 Use TypedDict or Dataclass Instead of dict[str, Any]
   • 3.10 Use isinstance() for Type Checking, Not hasattr/getattr
4. API Design — MEDIUM
   • 4.1 Avoid Boolean Flag Parameters in Public APIs
   • 4.2 Choose the Simplest Namespace That Matches Ownership and Polymorphism
   • 4.3 Don't Access Private Attributes
   • 4.4 Keep Data Models Flat and Non-Redundant
   • 4.5 Keep Old Names as Deprecated Aliases
   • 4.6 Order Required Fields Before Optional Fields
   • 4.7 Return New Collections from Transforms
   • 4.8 Underscore Prefix for Private Names
   • 4.9 Use Keyword-Only Parameters for Optional Config
5. Code Simplification — LOW-MEDIUM
   • 5.1 Extract Duplicated Logic Once Drift Risk Appears
   • 5.2 Flatten Nested if Statements Into and Conditions
   • 5.3 Inline Single-Use Intermediate Variables
   • 5.4 Remove Commented-Out and Dead Code
   • 5.5 Return Early to Flatten Control Flow
   • 5.6 Use @cached_property Only When the Instance Supports It
   • 5.7 Use Comprehensions Over for+append Loops
   • 5.8 Use any() / all() Over Boolean-Flag Loops
   • 5.9 Use x or default for Fallback Values
6. Performance — LOW-MEDIUM
   • 6.1 Build a Dict Index Instead of Nested Loops
   • 6.2 Combine Filter and Map Into One Pass
   • 6.3 Compile Static Regex Patterns at Module Level
   • 6.4 Define TypeAdapter Instances at Module Level
   • 6.5 Prefer Tuple Syntax in isinstance() Only on Profiled Hot Paths
   • 6.6 Stream with Generators When Memory or First-Result Latency Matters
   • 6.7 Use functools.lru_cache for Pure Functions
   • 6.8 Use set for Repeated Membership Checks
7. Naming — LOW-MEDIUM
   • 7.1 Avoid Redundant Type Suffixes in Names
   • 7.2 Drop Redundant Prefixes When Context Is Clear
   • 7.3 Rename When Behavior Changes
   • 7.4 Use Consistent Terminology Across Code and Docs
   • 7.5 Use Specific Parameter and Variable Names
   • 7.6 Use UPPER_CASE for Module Constants
8. Imports & Structure — LOW
   • 8.1 Handle Optional Dependencies Explicitly
   • 8.2 Keep Modules Cheap to Import
   • 8.3 No Duplicate Imports
   • 8.4 Place Imports at the Top of the File
   • 8.5 Remove Unused Imports
   • 8.6 Scope Helpers and Constants to Their Usage Site

1. Data Modeling

Impact: HIGH

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

1.1 Brand Primitive IDs With NewType

Impact: MEDIUM (catches ID-confusion bugs at type-check time)

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.

1.2 Create Explicit Variants Instead of Mode Flags

Impact: MEDIUM (eliminates conditional-logic sprawl)

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.

1.3 Delete Dead Variants

Impact: LOW-MEDIUM (removes code paths that can't be reached)

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

1.4 Derive, Don't Store

Impact: HIGH (eliminates flag-sync bugs and halves the state space)

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.

1.5 Encapsulate Mutable State in the Narrowest Clear Scope

Impact: MEDIUM (keeps mutation scoped to the owning object)

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.

1.6 Never Use Mutable Default Arguments

Impact: CRITICAL (prevents shared-state bugs across calls and instances)

A default argument is evaluated once, when the def 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.

1.7 Phase Related Optional Fields Into Nested Structs

Impact: MEDIUM (one optional check instead of eight)

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.

1.8 Pick a Mutation Contract

Impact: HIGH (prevents ambiguous caller expectations)

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.

1.9 Use Discriminated Unions Over Optional Bags

Impact: MEDIUM (tags variants explicitly in new designs; retrofit is expensive)

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.

1.10 Use Timezone-Aware Datetimes at Boundaries

Impact: HIGH (prevents off-by-hours bugs across timezones, daylight saving, and storage)

A datetime with no tzinfo is naive: 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.

1.11 Use a Sentinel Object When None Is a Real Domain Value

Impact: MEDIUM (distinguishes "no value passed" from "None passed deliberately")

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.

2. Error Handling

Impact: MEDIUM-HIGH

Specific exceptions, context managers for resources, preserved cancellation semantics. Sloppy exceptions hide bugs; narrow catches localize them.

2.1 Catch Specific Exception Types

Impact: HIGH (prevents masked bugs and broken Ctrl-C / cancellation)

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.

2.2 Consolidate try/except Blocks with the Same Handler

Impact: LOW-MEDIUM (reduces duplication and simplifies control flow)

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.

2.3 Inherit New Exceptions from Existing Base Exceptions

Impact: MEDIUM (preserves backward compatibility for callers)

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.

2.4 Preserve Tracebacks When Logging Exceptions

Impact: MEDIUM (keeps diagnostics intact when recovering from failures)

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.

2.5 Trust Validated State Within the Same Trust Domain

Impact: MEDIUM (removes clutter without losing real safety)

Once a value has been validated and the validated object is immutable, locally constructed, and stays 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.

2.6 Use !r Format for Identifiers in Error Messages

Impact: LOW (produces consistent, unambiguous 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.

2.7 Use assert Only for Debug-Only Internal Invariants

Impact: MEDIUM (-O strips asserts; use for debug-only invariants, not runtime contracts)

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.

2.8 Use assert_never for Exhaustiveness Checks

Impact: MEDIUM (turns "missing variant" into a type-check error)

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.

2.9 Use raise ... from to Preserve Exception Causality

Impact: LOW-MEDIUM (explicit __cause__ chain vs implicit __context__; often invisible to end callers)

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.

2.10 Use with / async with for Resource Lifetimes

Impact: HIGH (deterministic cleanup even on exceptions)

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.

2.11 Validate Input at System Boundaries

Impact: MEDIUM (fails fast and prevents bad data from spreading)

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.

3. Type Safety

Impact: MEDIUM-HIGH

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

3.1 Avoid Any Annotations

Impact: MEDIUM (narrows types in new code; retrofitting existing Any is churn)

Any turns off the type checker for that value — it accepts anything, produces anything, and propagates silently into every call site that consumes it. Any is common when the right type feels hard to write; almost always, a Protocol, TypeVar, or Union is available.

Incorrect (Any leaks through the system):

from typing import Any

def process_items(items: Any) -> Any:
    return [transform(item) for item in items]

def transform(item: Any) -> Any:
    return item.value.upper()

The checker cannot help here. A caller passing a dict instead of a list silently walks into runtime errors. item.value.upper() is unchecked — a typo in value would never be caught.

Correct (precise types; Protocol for duck-typed inputs):

from typing import Protocol

class HasValue(Protocol):
    value: str

def process_items(items: list[HasValue]) -> list[str]:
    return [transform(item) for item in items]

def transform(item: HasValue) -> str:
    return item.value.upper()

The checker now verifies that every call site passes a list of objects with a .value: str attribute. Typos in .value get caught. Return types propagate.

Correct (TypeVar for truly generic containers):

from typing import TypeVar

T = TypeVar("T")

def first_or_none(items: list[T]) -> T | None:
    return items[0] if items else None

Generic, not unchecked.

When Any is genuinely unavoidable (interop with dynamically typed libraries, some JSON boundaries), restrict its scope to one line, narrow to a concrete type immediately, and document the invariant in a comment.

3.2 Fix Type Definitions Instead of cast()

Impact: MEDIUM (keeps source type definitions honest)

cast(T, value) tells the checker to pretend value is a T with no runtime check. When called to paper over a structural mismatch, it hides a design problem. Reach for it only when runtime logic genuinely narrows in a way the checker can't express.

Incorrect (cast masks an unnecessarily wide return type):

from typing import cast

def load_config() -> dict[str, object]:
    return json.loads(CONFIG_PATH.read_text())

def get_timeout() -> int:
    config = load_config()
    return cast(int, config["timeout"])  # we're just telling the checker to trust us

The real issue: load_config returns dict[str, object] because json.loads does. But this project's config has a known shape — fix the source type.

Correct (declare the real structure):

from typing import TypedDict

class Config(TypedDict):
    timeout: int
    retries: int

def load_config() -> Config:
    return json.loads(CONFIG_PATH.read_text())  # validate or cast here, once

def get_timeout() -> int:
    config = load_config()
    return config["timeout"]  # known to be int from Config

Now every downstream consumer benefits from the typed shape.

When cast() is the right tool: when runtime logic narrows beyond what the checker can prove — e.g., after a literal tag check, a custom predicate, or a known invariant enforced elsewhere.

from typing import cast

def handle_success(result: ApiResponse) -> str:
    # An earlier check already verified result.status == "success"
    # but the checker can't propagate that narrowing here
    assert result.status == "success"
    return cast(SuccessResponse, result).data  # ok; narrowing is real

Even then, isinstance or a TypeGuard function is usually cleaner. Reserve cast for cases where those don't fit.

Rule of thumb: before reaching for cast, ask whether the source type should be narrower. 8 times out of 10, yes.

3.3 Fix Type Errors, Don't Ignore Them

Impact: HIGH (prevents masked errors from compounding)

# type: ignore and # pyright: ignore silence the checker — but the underlying problem stays. Ignore comments are common when a type looks hard; each one degrades the signal from every future run. Fix the error properly, and when a suppression is genuinely unavoidable, document why.

Incorrect (ignore comment masks the real problem):

def compute(items: list[int] | None) -> int:
    return sum(items)  # type: ignore  # noqa

The checker flagged this because sum(None) crashes at runtime. The ignore hides a real bug.

Correct (handle the None case):

def compute(items: list[int] | None) -> int:
    if items is None:
        return 0
    return sum(items)

The type system caught a real bug; fixing it is the right answer.

When a suppression is genuinely required (a complex generic the checker can't handle, a known checker limitation, an external library with bad stubs), include:

1. The specific error code (e.g., # type: ignore[arg-type])
2. A comment explaining the safety reasoning

# mypy cannot narrow through the factory's return type, but the
# registry guarantees adapters[cls] returns cls instances.
# See: https://github.com/python/mypy/issues/XXXX
adapter = adapters[cls]  # type: ignore[assignment]

A reviewer should be able to read the comment and confirm the suppression is justified without re-deriving the reasoning.

Escape hatches to prefer before ignoring:

• cast(T, value) with a comment (see types-fix-types-not-cast for when it's appropriate)
• assert isinstance(x, T) — runtime check plus narrowing
• TypeGuard functions for reusable narrowing
• Actually fixing the type signatures upstream

Reach for # type: ignore last, not first.

3.4 Narrow Type Signatures to Runtime Reality

Impact: MEDIUM (eliminates unreachable branches and false permissiveness)

If control flow (a match statement, an API contract, an earlier isinstance check) guarantees that only a subset of a union reaches a code path, the annotation should reflect that — not the wider union. Over-broad annotations create dead branches and suggest possibilities the code can't actually handle.

Incorrect (signature wider than reality):

def render_tool_result(part: MessagePart) -> str:
    # by contract this is only called with ToolResultPart or ToolCallPart
    if isinstance(part, ToolResultPart):
        return f"Result: {part.content}"
    if isinstance(part, ToolCallPart):
        return f"Call: {part.tool_name}"
    if isinstance(part, TextPart):
        return part.text  # unreachable — caller never passes TextPart
    raise ValueError(f"unexpected part: {part}")

The TextPart branch can't run (the caller guarantees it), but the type says MessagePart. Readers have to figure out the contract from context.

Correct (tighten the annotation):

ToolPart = ToolCallPart | ToolResultPart

def render_tool_result(part: ToolPart) -> str:
    if isinstance(part, ToolResultPart):
        return f"Result: {part.content}"
    return f"Call: {part.tool_name}"  # must be ToolCallPart

The signature documents the contract. No dead branches. A caller that tries to pass a TextPart gets a type error, not a runtime ValueError.

When the wider type is necessary: when the function genuinely handles the full union at some call sites and a subset at others, accept the widest used type and narrow inside. Don't invent a separate wider signature "to be safe."

Exhaustiveness check:

from typing import assert_never

def render_tool_result(part: ToolPart) -> str:
    match part:
        case ToolResultPart(): return f"Result: {part.content}"
        case ToolCallPart(): return f"Call: {part.tool_name}"
        case _: assert_never(part)  # checker fails if union grows

assert_never ensures that if ToolPart gains a new variant, every match on it is re-examined.

3.5 Remove Redundant | None When Values Are Guaranteed

Impact: LOW-MEDIUM (eliminates false uncertainty in the type signature)

An annotation of X | None tells readers and the checker that None is a real possibility — every consumer now writes a None check. When the value is guaranteed to be set (by the constructor, by the control flow, by an earlier validation), | None lies about the API.

Incorrect (optional annotation on a guaranteed-present value):

from dataclasses import dataclass

@dataclass
class Session:
    user_id: str
    token: str | None = None  # but we always generate a token in __post_init__

    def __post_init__(self) -> None:
        if self.token is None:
            self.token = generate_token()

Every consumer of session.token now writes if session.token is not None: ... — for a value that is always present.

Correct (use a factory default; drop the optional):

from dataclasses import dataclass, field

@dataclass
class Session:
    user_id: str
    token: str = field(default_factory=generate_token)

session.token is now unambiguously a str. No defensive None checks downstream.

Also incorrect (| None on NotRequired TypedDict fields):

from typing import TypedDict, NotRequired

class Config(TypedDict):
    name: str
    timeout: NotRequired[int | None]  # already optional via NotRequired

NotRequired already expresses "may be absent." Adding | None lets the caller pass None instead of omitting — which is rarely what you want. Either the field is absent (NotRequired) or it has a value (no | None).

When | None is correct: when None is a real, semantic value — "no assignee," "no parent," "not yet fetched." Absence as a meaningful state deserves None.

Heuristic: if every consumer writes if x is not None: before using the value, either None is never really set (remove | None) or you should have a different sentinel (a default, a distinct variant).

3.6 Trust the Type Checker — Remove Redundant Runtime Checks

Impact: LOW-MEDIUM (removes noise and signals confidence in the types)

When types already constrain a value, runtime checks for the same constraint add noise and imply the types aren't trustworthy. Every redundant assert or isinstance is a vote of no confidence in the rest of the type system.

Incorrect (runtime checks that duplicate the type):

def process_user(user: User) -> str:
    assert user is not None        # type says User, not User | None
    assert isinstance(user, User)   # type already says User
    assert user.name                # if name: str, this only catches empty strings
    return user.name.upper()

The first two lines add nothing. The third conflates "empty string" with "None" — if that matters, say so with a dedicated check and a real error message.

Correct (trust the signature):

def process_user(user: User) -> str:
    return user.name.upper()

If callers pass None against the signature, that's a bug in the caller — and the type checker will flag it at the call site.

Also incorrect (defensive check after validation):

def process_request(raw: str) -> Response:
    validated = validate(raw)  # returns ValidatedRequest, never None
    if validated is None:
        raise ValueError("invalid")  # unreachable
    return handle(validated)

validate returns ValidatedRequest by its signature — no None. The check is dead.

When runtime checks are the right tool:

• At trust boundaries: external API responses, deserialized user input, third-party callbacks where the type is aspirational
• As narrowing aids: assert isinstance(x, T) to narrow from a wider type the checker can't otherwise see
• For invariants the checker can't express: "this list is sorted," "this counter is positive"

Inside your own code, let the types do the work.

3.7 Use Literal Types for Fixed String Sets

Impact: MEDIUM (catches invalid strings at type-check time)

When a parameter accepts one of a fixed set of string values, str is too wide — every typo is legal. Literal["a", "b", "c"] narrows the type to exactly those values and enables match exhaustiveness checking.

Incorrect (plain str accepts anything):

def set_log_level(level: str) -> None:
    ...

set_log_level("DEUBG")  # typo — compiles fine, runtime surprise

The checker cannot tell you "DEUBG" is invalid. A typo at a call site silently passes through until the function hits an unexpected branch.

Correct (Literal restricts to the valid set):

from typing import Literal

LogLevel = Literal["DEBUG", "INFO", "WARNING", "ERROR"]

def set_log_level(level: LogLevel) -> None:
    ...

set_log_level("DEUBG")  # type error — caught at type-check time

IDEs autocomplete the valid values. Typos are flagged before running.

Pairs well with match:

def level_priority(level: LogLevel) -> int:
    match level:
        case "DEBUG": return 10
        case "INFO": return 20
        case "WARNING": return 30
        case "ERROR": return 40

If a new level is added to the Literal type without updating this match, checkers with exhaustiveness support flag the missing case.

When to use Enum instead: when the values have methods or rich behavior (e.g., LogLevel.DEBUG.name, LogLevel.DEBUG.value). Enums also work well with exhaustiveness checking but carry more ceremony than Literal. For plain string tags, Literal is lighter.

When to use plain str: free-form user input, message bodies, URLs, any field that isn't a finite enumeration.

3.8 Use TYPE_CHECKING for Optional Dependencies

Impact: LOW-MEDIUM (preserves type hints without forcing the import)

When a module's type hints reference a class from an optional dependency, importing the module should not require that dependency to be installed. if TYPE_CHECKING: blocks let the checker see the import while the runtime stays lean.

Incorrect (importing an optional dep at runtime):

import anthropic  # crashes if anthropic is not installed

class AnthropicProvider:
    def __init__(self, client: anthropic.Client) -> None:
        self._client = client

A user installing just the core package and never touching Anthropic still gets a ModuleNotFoundError at import time.

Incorrect (falling back to Any):

from typing import Any

class AnthropicProvider:
    def __init__(self, client: Any) -> None:  # we gave up on the type
        self._client = client

This works at runtime but loses type safety on every method that uses self._client.

Correct (TYPE_CHECKING block + quoted hint):

from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    import anthropic

class AnthropicProvider:
    def __init__(self, client: anthropic.Client) -> None:
        self._client = client

With from __future__ import annotations, all annotations are strings at runtime — so anthropic.Client in the signature doesn't need the import to resolve. The checker still resolves it during type-check because it sees the TYPE_CHECKING branch.

Pattern for optional-dep packages:

# At module top
try:
    import anthropic
except ImportError as e:
    raise ImportError(
        "Please install the anthropic extra: `pip install 'mylib[anthropic]'`"
    ) from e

Combine the runtime guard (helpful error if the user hits a code path that needs the dep) with TYPE_CHECKING for the hints.

3.9 Use TypedDict or Dataclass Instead of dict[str, Any]

Impact: MEDIUM (structured alternative to dict[str, Any] when writing new code)

When the shape of a dict is known — config objects, API payloads, structured events — dict[str, Any] is a lie about the data. The structure exists; it's just not declared, so every access is a runtime gamble. TypedDict or dataclass restores type-checker coverage.

Incorrect (structure erased):

from typing import Any

def create_user(config: dict[str, Any]) -> User:
    name = config["name"]            # what type?
    age = config.get("age", 0)       # what type?
    prefs = config.get("prefs", {})  # dict or the passed-in value?
    return User(name=name.upper(), age=age + 1, prefs=prefs)

Correct (TypedDict — dict-shaped but typed; use at serialization boundaries):

from typing import TypedDict, NotRequired

class UserConfig(TypedDict):
    name: str
    age: NotRequired[int]
    prefs: NotRequired["UserPreferences"]

def create_user(config: UserConfig) -> User:
    return User(name=config["name"].upper(), age=config.get("age", 0))

For in-memory values with behavior and defaults, prefer a dataclass; when you also need runtime validation, pydantic.BaseModel. dict[str, Any] is the right answer only for genuinely unstructured data — log context, free-form metadata. If you know the fields, declare them.

3.10 Use isinstance() for Type Checking, Not hasattr/getattr

Impact: MEDIUM (enables proper type narrowing for the checker)

Type checkers narrow types through isinstance() checks, discriminator match statements, and TypeGuard functions — not through hasattr(), getattr(), or type(obj).__name__ == "...". hasattr is common for "flexibility"; the actual cost is that the checker can't narrow and refactors silently break string comparisons.

Incorrect (hasattr/getattr defeats type narrowing):

def process(part: MessagePart) -> str:
    if hasattr(part, "tool_name"):
        return f"Tool: {part.tool_name}"  # type checker: attribute is Any
    if getattr(part, "kind", None) == "text":
        return part.text  # type checker: does part.text exist? unclear
    if type(part).__name__ == "ImagePart":
        return f"Image: {part.url}"  # fragile: renaming ImagePart breaks this
    return "unknown"

The checker gives up on every branch. If ToolPart is renamed, the type(...).__name__ string comparison silently stops matching — and no tests catch it because the function still runs.

Correct (isinstance enables narrowing):

def process(part: MessagePart) -> str:
    if isinstance(part, ToolPart):
        return f"Tool: {part.tool_name}"  # narrowed to ToolPart
    if isinstance(part, TextPart):
        return part.text                    # narrowed to TextPart
    if isinstance(part, ImagePart):
        return f"Image: {part.url}"         # narrowed to ImagePart
    return "unknown"

Now the checker verifies that part.tool_name, part.text, and part.url each exist on the narrowed type. Renaming a class triggers type errors at every use site.

For tagged unions, use match on the discriminator:

def process(part: MessagePart) -> str:
    match part.kind:
        case "tool":  return f"Tool: {part.tool_name}"
        case "text":  return part.text
        case "image": return f"Image: {part.url}"

When part.kind is a Literal discriminator on a Union, match narrows each branch to the matching variant.

When to reach for hasattr: genuinely optional extension protocols where classes may or may not implement a method. Even then, prefer isinstance(obj, Protocol) with a runtime_checkable Protocol over raw attribute probing.

4. API Design

Impact: MEDIUM

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

4.1 Avoid Boolean Flag Parameters in Public APIs

Impact: MEDIUM (prevents call sites that read like "do_thing(thing, True, False)")

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.

4.2 Choose the Simplest Namespace That Matches Ownership and Polymorphism

Impact: LOW-MEDIUM (avoids unnecessary coupling without forcing a binary choice)

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

4.3 Don't Access Private Attributes

Impact: LOW-MEDIUM (internals are free to change without notifying external callers)

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

4.4 Keep Data Models Flat and Non-Redundant

Impact: MEDIUM (reduces API surface and prevents field drift)

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.

4.5 Keep Old Names as Deprecated Aliases

Impact: MEDIUM (enables gradual migration without breakage)

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

4.6 Order Required Fields Before Optional Fields

Impact: HIGH (Python enforces this at class-definition time)

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

4.7 Return New Collections from Transforms

Impact: MEDIUM (prevents surprising side effects)

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.

4.8 Underscore Prefix for Private Names

Impact: LOW-MEDIUM (signals internal API and limits backward-compat obligations)

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.

4.9 Use Keyword-Only Parameters for Optional Config

Impact: MEDIUM (prevents breakage when adding or reordering params)

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.

5. Code Simplification

Impact: LOW-MEDIUM

Python idioms. Comprehensions, any()/all(), early returns. Mostly stylistic — apply when writing or reviewing.

5.1 Extract Duplicated Logic Once Drift Risk Appears

Impact: MEDIUM (prevents divergent implementations of the same logic)

The first copy is fine. The second copy is the decision point: extract now, or accept drift later. A third copy is the safe default for "rule of three," but if the logic encodes a domain rule (validation, formatting, permissions) that must stay consistent, extract at the second copy. The cost of delay is bugs where copies evolved in subtly different directions.

Incorrect (same logic duplicated across handlers):

def handle_stream(stream: AsyncIterator[Chunk]) -> Response:
    collected = []
    async for chunk in stream:
        if chunk.kind == "text":
            collected.append(chunk.text)
        elif chunk.kind == "tool":
            collected.append(format_tool(chunk))
    return Response(content="".join(collected))

def handle_non_stream(response: RawResponse) -> Response:
    collected = []
    for chunk in response.chunks:
        if chunk.kind == "text":
            collected.append(chunk.text)
        elif chunk.kind == "tool":
            collected.append(format_tool(chunk))
    return Response(content="".join(collected))

Two copies of the chunk-formatting logic. A new chunk kind gets added — two places need updates. One gets missed, and the streaming handler silently produces different output than the non-streaming one.

Correct (extract once, use twice):

def _format_chunk(chunk: Chunk) -> str:
    match chunk.kind:
        case "text": return chunk.text
        case "tool": return format_tool(chunk)
        case _: return ""

async def handle_stream(stream: AsyncIterator[Chunk]) -> Response:
    parts = [_format_chunk(c) async for c in stream]
    return Response(content="".join(parts))

def handle_non_stream(response: RawResponse) -> Response:
    parts = [_format_chunk(c) for c in response.chunks]
    return Response(content="".join(parts))

Now a new chunk kind means one change. The two handlers can't drift apart.

When NOT to extract:

• The two occurrences look similar but serve genuinely different purposes — premature abstraction locks them together
• The shared logic is 2-3 trivial lines and naming a helper is more noise than value
• Each caller would need the helper to accept so many optional parameters that it becomes a mode-switch

Location of the helper:

• Private (_name) in the same module if it's specific to that module
• Module-level utility if multiple modules share it
• Base class method if it's a shared class operation (data-explicit-variants)

5.2 Flatten Nested if Statements Into and Conditions

Impact: LOW (reduces indentation and improves readability)

When nested if statements share the same body with no intervening code, collapse them into a single if with and. The nested form implies the branches do something different; when they don't, the structure lies.

Incorrect (nested if with no intervening code):

def should_notify(user: User, event: Event) -> bool:
    if user.is_active:
        if user.notifications_enabled:
            if event.priority >= user.notification_threshold:
                return True
    return False

Correct (one combined condition):

def should_notify(user: User, event: Event) -> bool:
    return (
        user.is_active
        and user.notifications_enabled
        and event.priority >= user.notification_threshold
    )

Keep the nesting when intermediate logging, validation, or early returns happen between checks, or when the branches do genuinely different work. The rule: if every nested branch only holds another if until the final body, flatten. For chains where the happy path is the final action, the guard-clause form from simplify-early-return often reads cleanest.

5.3 Inline Single-Use Intermediate Variables

Impact: LOW (reduces noise and indirection)

When a variable is assigned once and used once immediately after, inlining it removes a name that doesn't earn its keep. Intermediates like _filtered, _cleaned, _copy show up "for clarity" — but the clarity is usually from the name, and if the name isn't informative, the variable is just noise.

Incorrect (intermediates that add nothing):

def top_admins(users: list[User], limit: int) -> list[User]:
    filtered_users = [u for u in users if u.is_admin]
    sorted_users = sorted(filtered_users, key=lambda u: u.rank)
    result = sorted_users[:limit]
    return result

Four lines, four names — each used exactly once. The names restate the operations, not the purpose.

Correct (inline when the operation is its own explanation):

def top_admins(users: list[User], limit: int) -> list[User]:
    return sorted(
        (u for u in users if u.is_admin),
        key=lambda u: u.rank,
    )[:limit]

Four operations, zero intermediate names. Each step's intent is visible in place.

When an intermediate variable earns its place:

• The name adds genuine information the expression doesn't convey
• The value is used more than once
• The expression would be too long to read as one unit
• Debugging benefits from a named step (breakpoints, logging)

# the name "eligible" adds information the expression doesn't
def distribute_bonuses(users: list[User], amount: Decimal) -> None:
    eligible = [u for u in users if u.tenure_months >= 12 and not u.on_leave]
    share = amount / len(eligible)
    for user in eligible:
        pay_bonus(user, share)

Here eligible is used twice and the name documents the business rule. Keep it.

Heuristic: if you can't give the intermediate a name more meaningful than a restatement of the operation, inline it.

5.4 Remove Commented-Out and Dead Code

Impact: LOW-MEDIUM (reduces confusion about intent)

Commented-out code, superseded implementations, unused imports, and definitions nothing calls — delete them. Version control preserves history; dead code in the file confuses readers about which implementation is actually active.

Incorrect (commented alternatives and stale code):

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

    response = http_client.get(f"/users/{user_id}")
    return User.model_validate(response.json())

    # TODO: switch to async version once available
    # async def fetch_user(user_id): ...

def _unused_helper(x: int) -> int:  # nothing calls this
    return x * 2

Readers wonder: is the commented version the fallback? Is the TODO still accurate? Is _unused_helper called dynamically somewhere I'm missing?

Correct (delete; let git remember):

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

If you need the old implementation back, git log has it.

Targets to delete:

• Commented-out blocks of code
• Functions, methods, classes that nothing calls (verify with grep across the codebase first)
• Unused imports (ruff or pyflakes finds these)
• Unused variables
• Dead branches (if a condition can never be true)
• print() statements left over from debugging
• Commented-out log lines

When to keep what looks like dead code:

• Called dynamically (by name, via getattr, via a registry)
• Part of a public API contract that external callers may use
• Intentionally present for future use, with a TODO that links to a tracking issue

For "intentional TODOs," link to the tracking issue:

# TODO(ISSUE-123): switch to async http_client when the migration lands

A TODO with a link is a commitment. A TODO without one is a wish.

5.5 Return Early to Flatten Control Flow

Impact: LOW-MEDIUM (keeps the happy path unnested and readable)

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.

5.6 Use @cached_property Only When the Instance Supports It

Impact: MEDIUM (defers work safely; misuse causes races and silent staleness)

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

5.7 Use Comprehensions Over for+append Loops

Impact: LOW (more concise, often faster, and idiomatic Python)

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.

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

Impact: LOW (shorter, short-circuits, no manual flag management)

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.

5.9 Use x or default for Fallback Values

Impact: LOW (more concise and idiomatic than if/else)

For the common "use x if it's truthy, otherwise default" pattern, x or default beats the verbose if/else. The catch: this triggers on every falsy value (0, '', [], None) — so only use it when those aren't semantically meaningful.

Incorrect (verbose if/else for a simple fallback):

def display_name(user: User) -> str:
    if user.nickname:
        return user.nickname
    else:
        return user.username

Correct (or-fallback):

def display_name(user: User) -> str:
    return user.nickname or user.username

Shorter, idiomatic, and clear.

When or is WRONG: when falsy values are semantically valid.

# wrong — if count is 0, we'd return DEFAULT_COUNT instead of 0
def get_count(config: Config) -> int:
    return config.count or DEFAULT_COUNT

# right — explicit about the None case
def get_count(config: Config) -> int:
    return config.count if config.count is not None else DEFAULT_COUNT

Zero, empty string, empty list, and empty dict are all falsy but often meaningful. 0 retries ≠ default retries. "" name is probably a bug but it's not the same as "name was never set."

Use ... if ... is not None else ... when you specifically mean "None":

timeout = config.timeout if config.timeout is not None else DEFAULT_TIMEOUT

Use ... if ... else ... when the condition is more elaborate:

label = name.strip() if name and name.strip() else "anonymous"

Rule of thumb: or for truthy/falsy semantics; explicit is not None for optional-value semantics.

6. Performance

Impact: LOW-MEDIUM

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

6.1 Build a Dict Index Instead of Nested Loops

Impact: MEDIUM (O(n) instead of O(n²))

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.

6.2 Combine Filter and Map Into One Pass

Impact: LOW (one iteration instead of two or three)

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.

6.3 Compile Static Regex Patterns at Module Level

Impact: LOW (marginal outside tight loops; Python's re cache handles most cases)

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.

6.4 Define TypeAdapter Instances at Module Level

Impact: LOW-MEDIUM (avoids repeated schema construction)

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.

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

Impact: LOW (tiny per-call savings; only relevant in tight loops)

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.

6.6 Stream with Generators When Memory or First-Result Latency Matters

Impact: LOW-MEDIUM (bounded memory and lazy evaluation for large or infinite sequences)

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.

6.7 Use functools.lru_cache for Pure Functions

Impact: LOW-MEDIUM (trades memory for CPU on repeatable computations)

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.

6.8 Use set for Repeated Membership Checks

Impact: MEDIUM (O(1) beats O(n))

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.

7. Naming

Impact: LOW-MEDIUM

Names carry meaning. Specific over generic, consistent terminology, no type suffixes. Mostly applies to new names.

7.1 Avoid Redundant Type Suffixes in Names

Impact: LOW (reduces noise when types annotate types)

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.

7.2 Drop Redundant Prefixes When Context Is Clear

Impact: LOW-MEDIUM (reduces noise and improves readability)

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.

7.3 Rename When Behavior Changes

Impact: MEDIUM (prevents misleading names from hiding 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.

7.4 Use Consistent Terminology Across Code and Docs

Impact: MEDIUM (prevents fragmented searches and user confusion)

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.

7.5 Use Specific Parameter and Variable Names

Impact: LOW-MEDIUM (prevents confusion when multiple instances are in scope)

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.

7.6 Use UPPER_CASE for Module Constants

Impact: LOW (signals immutability and public/private scope)

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.

8. Imports & Structure

Impact: LOW

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

8.1 Handle Optional Dependencies Explicitly

Impact: LOW-MEDIUM (clear error messages instead of cryptic ImportError)

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.

8.2 Keep Modules Cheap to Import

Impact: MEDIUM (faster CLIs, faster tests, faster worker startup)

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.

8.3 No Duplicate Imports

Impact: LOW (prevents confusion and redundant work)

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.

8.4 Place Imports at the Top of the File

Impact: LOW (makes dependencies visible at a glance)

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.

8.5 Remove Unused Imports

Impact: LOW (prevents accidental dependencies and reduces noise)

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.

8.6 Scope Helpers and Constants to Their Usage Site

Impact: LOW (reduces namespace pollution and clarifies intent)

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.

References

• https://docs.python.org/3/library/typing.html
• https://docs.python.org/3/library/dataclasses.html
• https://docs.python.org/3/library/exceptions.html
• https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement
• https://docs.pydantic.dev/
• https://mypy.readthedocs.io/
• https://docs.astral.sh/ruff/
• https://peps.python.org/pep-0008/
• https://peps.python.org/pep-0544/
• https://peps.python.org/pep-0604/
• https://peps.python.org/pep-0615/
• https://peps.python.org/pep-0661/
• https://peps.python.org/pep-0695/
• https://peps.python.org/pep-0702/
• https://github.com/pydantic/pydantic-ai