---
title: "Python pack"
description: "Draft cards specific to python, not vetted. Use them with the level page that matches your input."
canonical: "https://goodcodereview.com/pack-python"
updated: "2026-10-25"
---

# Python pack

A draft card is not vetted. Read it for ideas, but do not use it for a verdict. Draft cards are never verdicts.

## py-await-check-then-act

severity: block  
concerns: correctness  
applies_when: An `async def` reads state, awaits, then writes based on that read.

**Rule.** When more than one worker or process can run, make check-then-act an atomic DB statement or constraint, because a Python lock covers one process only.

**Why.** Core rule: [fn-no-check-then-act-across-await](https://goodcodereview.com/function.html#fn-no-check-then-act-across-await). An `asyncio.Lock` does serialise tasks inside one event loop, but a second worker or host shares no lock, so the guard has to live where the state lives.

**FAIL**

```python
async with lock:                       # fine for one process, useless with two workers
    bal = await repo.get_balance(acct)
    if bal >= amt:
        await repo.set_balance(acct, bal - amt)
```

**PASS**

```python
result = await session.execute(
    update(Account)
    .where(Account.id == acct, Account.balance >= amt)
    .values(balance=Account.balance - amt)
)
if result.rowcount == 0:
    raise InsufficientFunds(acct)
```

**Counterpoint.** "asyncio is single-threaded, so a lock or no lock is enough." Within one process, an `asyncio.Lock` is correct, and without it every await is a point where another task runs. The rule applies once deployment can run more than one worker, which is the normal case.

## py-cancellation

severity: block  
concerns: reliability, correctness  
applies_when: Async code catches exceptions around awaits or uses asyncio.timeout.

**Rule.** Re-raise `CancelledError` (do not catch it via bare `except` or `BaseException`), and catch `TimeoutError` outside the `asyncio.timeout()` block.

**Why.** TaskGroup and timeouts are built on cancellation, so swallowing it makes shutdown hang and timeouts silently not fire. The conversion to `TimeoutError` happens at context exit, so a handler inside the block never sees it.

**FAIL**

```python
async with asyncio.timeout(5):
    try: await fetch()
    except TimeoutError: ...        # never reached
    except BaseException: pass      # eats cancellation
```

**PASS**

```python
try:
    async with asyncio.timeout(5):
        await fetch()
except TimeoutError:
    handle_timeout()
```

**Counterpoint.** "Cleanup code must run even on cancel." Clean up in `finally`, then re-raise, and never suppress. `uncancel()` is task bookkeeping, not permission to swallow.

## py-class-earns-its-instance

severity: discuss  
concerns: maintainability  
applies_when: A Python class has no state, only static methods, or a `Protocol` with one implementation.

**Rule.** Give a class real state or a collaborator; otherwise write a module function.

**Why.** A static-only class is a module with extra typing. A `Protocol` with one implementation is a speculative interface. A `Protocol` needs two or more real implementations swapped at runtime; see [mod-yagni](https://goodcodereview.com/module.md#mod-yagni).

**FAIL**

```python
class DateUtils:
    @staticmethod
    def parse(s): ...
class Notifier(Protocol): ...   # one impl, ever
```

**PASS**

```python
dates.py:  def parse(s): ...
# use the concrete class until a second real impl exists
```

**Counterpoint.** "Protocols make testing easy." A fake of a concrete class in tests works with duck typing too. The Go review guide takes the same stance: declare interfaces where consumed, and avoid speculative ones.

## py-docstrings-google

severity: discuss  
concerns: maintainability  
applies_when: A Python function or class has, or lacks, a docstring.

**Rule.** Write a Google-style docstring only when it adds what the signature cannot say.

**Why.** Forced docstrings restate the signature and rot. The ones that matter state units, side effects, and what callers may rely on. The gate never forces presence; it checks format only.

**FAIL**

```python
def add(a, b):
    """Add a and b. :param a: first number"""
```

**PASS**

```python
def charge(order):
    """Charge the card once; safe to retry with the same order id (idempotent)."""
```

**Counterpoint.** "Every public function needs a docstring." Ousterhout would sympathise (missing docs cost more than wrong ones), and Martin would not. The compromise here is to require content that adds information, not presence.

## py-error-hierarchy

severity: should-fix  
concerns: correctness  
applies_when: A Python package raises its own exceptions.

**Rule.** Give each package one base exception, with the specific ones in an `errors` module.

**Why.** A base class gives callers one safe catch for the package. An inline `class X(Exception)` next to a raise scatters the hierarchy. Chain with `raise ... from err` ([fn-propagate-dont-swallow](https://goodcodereview.com/function.html#fn-propagate-dont-swallow)).

**FAIL**

```python
raise Exception("declined")
```

**PASS**

```python
# billing/errors.py
class BillingError(Exception): ...
class PaymentDeclined(BillingError): ...
```

**Counterpoint.** "Built-in exceptions are enough." For ValueError-like cases, yes. For domain failures, callers need names to catch and a base to catch them all.

## py-field-only-when-adds

severity: discuss  
concerns: correctness  
applies_when: A pydantic model or settings class uses `Field()`.

**Rule.** Use `Field()` only when it adds something (a constraint, an alias, or API docs), and set plain defaults directly.

**Why.** `Field(default=5.0)` is noise that hides which fields are constrained.

**FAIL**

```python
request_timeout_s: float = Field(default=5.0)
```

**PASS**

```python
request_timeout_s: float = 5.0
max_batch_size: int = Field(default=500, gt=0, le=2_000)
```

**Counterpoint.** "Uniform `Field` everywhere is consistent." Consistency that hides the constraints is not worth the scan cost. Every `Field` should tell the reader something.

## py-frozen-dataclass-over-tuple

severity: should-fix  
concerns: correctness  
applies_when: A Python function returns a tuple whose positions differ in meaning.

**Rule.** Return a `@dataclass(frozen=True, slots=True)` instead of a positional tuple.

**Why.** Core rule: [fn-no-anonymous-tuples](https://goodcodereview.com/function.html#fn-no-anonymous-tuples). In Python, the frozen dataclass also gives hashing, immutability, and a compact `slots` layout.

**FAIL**

```python
def parse_error(exc) -> tuple[str, str]: ...
```

**PASS**

```python
@dataclass(frozen=True, slots=True)
class ParsedError:
    code: str
    message: str
```

**Counterpoint.** "`NamedTuple` is lighter." Fine; both name the fields. Use the one your team already uses, but never a bare tuple.

## py-httpx-client-lifespan

severity: should-fix  
concerns: performance  
applies_when: A Python service creates an `httpx` client.

**Rule.** Pass an explicit `httpx.Timeout` to every `httpx` client.

**Why.** Core rules: [svc-timeouts](https://goodcodereview.com/service.md#svc-timeouts), [mod-pooled-clients-built-once](https://goodcodereview.com/module.md#mod-pooled-clients-built-once). `read=None` disables the read timeout. Build the client once in the app lifespan and reuse it.

**FAIL**

```python
async def fetch(url):
    async with httpx.AsyncClient(timeout=None) as c:
        return await c.get(url)
```

**PASS**

```python
@asynccontextmanager
async def lifespan(app):
    async with httpx.AsyncClient(timeout=httpx.Timeout(10.0, connect=2.0)) as c:
        app.state.http = c
        yield
```

**Counterpoint.** "httpx defaults are fine." They are (5s), but set the value explicitly so that a library change cannot silently change behaviour.

## py-imports

severity: should-fix  
concerns: maintainability  
applies_when: A Python module has imports.

**Rule.** Put imports at module top, absolute only.

**Why.** Local imports hide dependencies and cycles. Relative imports break when files move. Importing a private name couples you to something its owner may change without notice. Never import a `_`-prefixed name from another module, and use `__all__` only in a re-export `__init__.py`.

**FAIL**

```python
def handler():
    from .helpers import _parse
```

**PASS**

```python
from app.billing.parsing import parse
```

**Counterpoint.** "Import inside the function avoids a cycle." It hides the cycle instead of fixing it, and the design needs a third module or a dependency inversion. A lazy import for a heavy optional dependency is the one defensible exception; document it with a reason.

## py-init-assign-only

severity: should-fix  
concerns: maintainability  
applies_when: A Python class defines `__init__` or `__post_init__`.

**Rule.** Keep `__init__` and `__post_init__` to assignments, and do I/O in a `@classmethod` factory.

**Why.** Core rule: [mod-no-logic-in-constructors](https://goodcodereview.com/module.html#mod-no-logic-in-constructors). A dataclass's `__post_init__` is a constructor too, so the same rule applies there.

**FAIL**

```python
@dataclass
class PriceList:
    path: Path
    def __post_init__(self):
        self.prices = json.loads(self.path.read_text())
```

**PASS**

```python
@dataclass
class PriceList:
    prices: dict[str, Decimal]

    @classmethod
    def from_file(cls, path: Path) -> "PriceList":
        raw = json.loads(path.read_text())
        return cls({sku: Decimal(p) for sku, p in raw.items()})
```

**Counterpoint.** "Dataclasses make `__post_init__` normal." It is normal for derived fields and cheap validation, but not for reading files or calling APIs.

## py-init-py-empty

severity: should-fix  
concerns: maintainability  
applies_when: The change adds or edits an `__init__.py`.

**Rule.** Keep `__init__.py` empty unless it re-exports the public API.

**Why.** Logic in `__init__.py` runs at import ([mod-no-import-side-effects](https://goodcodereview.com/module.html#mod-no-import-side-effects)) and hides where things live. A re-export file may use `__all__`. An empty one has no comment or docstring.

**FAIL**

```python
# __init__.py
"""Utils."""
engine = create_engine(...)
```

**PASS**

```python
# __init__.py  (empty)
# or, at a library boundary:
from mylib.client import Client
__all__ = ["Client"]
```

**Counterpoint.** "A package docstring helps." Put it in the README or docs. An empty `__init__.py` says clearly that nothing is hidden here.

## py-modern-typing

severity: should-fix  
concerns: correctness  
applies_when: Annotations use `typing` aliases, or the type gate is missing.

**Rule.** Write `X | None` and builtin generics, not `typing` aliases.

**Why.** A tool decides these, so a human comment is waste. Check that the gate exists, and gate new code with `mypy --strict` and pyright.

**FAIL**

```python
from typing import Optional, List
def f(x: Optional[List[int]]): ...
```

**PASS**

```python
def f(x: list[int] | None) -> None: ...
```

**Counterpoint.** "Two type checkers is overkill." They disagree on edge cases, so running both on new code catches different mistakes. Pick at least one as a hard gate.

## py-no-cast

severity: should-fix  
concerns: correctness  
applies_when: Code calls `typing.cast(...)`.

**Rule.** Do not use `cast()`; parse the boundary into a typed model, or narrow with `isinstance`.

**Why.** `cast` is a promise to the type checker that nothing verifies at runtime. When the promise is wrong, the type checker's output is a lie that spreads.

**FAIL**

```python
data = cast(dict[str, int], json.loads(raw))
```

**PASS**

```python
class Payload(BaseModel):
    counts: dict[str, int]
data = Payload.model_validate_json(raw)
```

**Counterpoint.** "Sometimes the checker is wrong." Then fix the stub or narrow with `isinstance` or `assert`, which check at runtime. A justified `type: ignore[code]` with a reason is more honest than a cast, because it stays visible.

## py-no-init-or-model-tests

severity: should-fix  
concerns: testing  
applies_when: The diff adds `test_init_*` or a test file for a plain model or dataclass.

**Rule.** Cover `__init__` and plain models through the behaviour that uses them.

**Why.** Such tests restate the class and break on every field change without proving behaviour.

**FAIL**

```python
def test_init_sets_name(): assert User(name="a").name == "a"
```

**PASS**

```python
def test_welcome_email_uses_user_name(): ...
```

**Counterpoint.** "Validators on the model need tests." Yes: test the validator's behaviour (valid and invalid input), not that assignment works.

## py-no-silenced-diagnostics

severity: should-fix  
concerns: maintainability  
applies_when: A diff adds `# noqa` or `# type: ignore`.

**Rule.** Suppress a Python diagnostic only with a single named code and a reason on the same line.

**Why.** Core rule: [chg-no-silenced-diagnostics](https://goodcodereview.com/change.html#chg-no-silenced-diagnostics). A bare `# type: ignore` silences every error on the line, including the next one someone introduces.

**FAIL**

```python
total = compute(order)  # type: ignore
x = risky()  # noqa
```

**PASS**

```python
import numpy_stubs  # type: ignore[import-untyped]  # no stubs published upstream
except Exception:  # noqa: BLE001  # process boundary: log and continue
```

**Counterpoint.** "A reason on every line is noise." Only suppressions need one, and they should be rare. A pile of them is the finding.

## py-paths-from-file

severity: should-fix  
concerns: correctness  
applies_when: Python code builds a path to a data file or fixture.

**Rule.** Resolve paths from `__file__`, not the current working directory.

**Why.** A cwd-relative path works from the repo root and fails from CI, an IDE, or an installed package.

**FAIL**

```python
Path("tests/fixtures/data.json")
```

**PASS**

```python
Path(__file__).parent / "fixtures" / "data.json"
```

**Counterpoint.** "Tests always run from the root." That holds until someone runs one test from its directory, or an installer moves it.

## py-pydantic-validation

severity: should-fix  
concerns: correctness  
applies_when: A Python function validates request or config data by hand.

**Rule.** Validate request data with a pydantic model, not hand-written checks.

**Why.** Core rule: [mod-validate-at-boundary-once](https://goodcodereview.com/module.html#mod-validate-at-boundary-once). Fields stay `snake_case` in Python. For camelCase JSON, use an alias generator and dump by alias.

**FAIL**

```python
def create_order(body: dict):
    if body.get("currency") not in {"EUR", "USD"}:
        raise ValueError("bad currency")
```

**PASS**

```python
from pydantic import BaseModel, ConfigDict, Field
from pydantic.alias_generators import to_camel

class CreateOrder(BaseModel):
    model_config = ConfigDict(alias_generator=to_camel, populate_by_name=True)
    items: list[LineItemIn] = Field(min_length=1)
    currency: Currency          # StrEnum: unknown values fail at parse

body = CreateOrder.model_validate(payload)
body.model_dump(by_alias=True)
```

**Counterpoint.** "Validators hide logic." They put it next to the field it protects. Hand-rolled checks are where drift starts.

## py-pytest-parametrize

severity: should-fix  
concerns: testing  
applies_when: Tests differ only in inputs and expected values.

**Rule.** Use `@pytest.mark.parametrize` for test variations.

**Why.** Copy-pasted tests drift, and an empty parametrization silently runs nothing. Assert that a computed parameter list is not empty first.

**FAIL**

```python
def test_retry_502(): assert is_retryable(Response(502))
def test_retry_503(): assert is_retryable(Response(503))
```

**PASS**

```python
@pytest.mark.parametrize(("status", "retry"), [(502, True), (503, True), (400, False)])
def test_retries_gateway_errors(status, retry):
    assert is_retryable(httpx.Response(status)) is retry
```

**Counterpoint.** "Separate tests give clearer failures." Parametrize ids give one failure per case, with its inputs. For a computed list, `assert cases` before parametrising.

## py-ruff-gate

severity: should-fix  
concerns: maintainability  
applies_when: The project is Python and has a linter configuration.

**Rule.** Enable the Ruff rule sets E, F, I, B, UP, SIM, BLE, TID and D in CI.

**Why.** The tool owns formatting, import order, the choice between `Optional` and `X | None`, bare `except`, and relative imports. Human review time goes to what tools cannot see. `D1xx` stays ignored: the gate checks docstring format, not presence. Anything the gate decides is not a review comment; the comment is "add the gate".

**FAIL**

```toml
[tool.ruff.lint]
select = ["E", "F"]
```

**PASS**

```toml
[tool.ruff.lint]
select = ["E", "F", "I", "B", "UP", "SIM", "BLE", "TID", "D"]
ignore = ["D100", "D101", "D102", "D103", "D104", "D105", "D106", "D107"]
[tool.ruff.lint.pydocstyle]
convention = "google"
[tool.ruff.lint.flake8-tidy-imports]
ban-relative-imports = "all"
```

**Counterpoint.** "Strict lint slows contributors." It speeds review: repeated nitpicks are a sign of a missing gate (Google, Pragmatic Engineer notes).

## py-settings-pydantic-settings

severity: should-fix  
concerns: maintainability  
applies_when: Python code reads configuration from the environment.

**Rule.** Load env config with `BaseSettings` from `pydantic_settings`.

**Why.** Core rule: [mod-central-config](https://goodcodereview.com/module.html#mod-central-config). `os.environ.get` returns untyped strings with silent defaults, while `BaseSettings` parses, validates, and fails at startup. In pydantic v2, `BaseSettings` is no longer importable from `pydantic`.

**FAIL**

```python
timeout = float(os.environ.get("TIMEOUT", "5"))
```

**PASS**

```python
from pydantic_settings import BaseSettings

class Settings(BaseSettings):
    database_url: str
    request_timeout_s: float = 5.0
```

**Counterpoint.** "os.environ is stdlib and enough." For one flag, maybe. Past two, you re-implement parsing and error messages at every call site.

## py-strenum-not-strings

severity: should-fix  
concerns: correctness  
applies_when: Python code compares against string or number literals for a fixed set of values.

**Rule.** Use `StrEnum` for fixed string sets and `HTTPStatus` for HTTP codes.

**Why.** Core rules: [fn-no-magic-values](https://goodcodereview.com/function.html#fn-no-magic-values), [fn-no-stringly-typed-returns](https://goodcodereview.com/function.html#fn-no-stringly-typed-returns). `StrEnum` members are still `str` (JSON-ready, comparable), and a pydantic field typed with one rejects unknown values at parse time.

**FAIL**

```python
if resp.status_code == 404 or mode == "fast": ...
```

**PASS**

```python
class Mode(StrEnum):
    FAST = "fast"

if resp.status_code == HTTPStatus.NOT_FOUND or mode == Mode.FAST: ...
```

**Counterpoint.** "An enum is heavy for two values." It is one class, and it makes the type checker guard every caller. `StrEnum` needs Python 3.11 or later; on older versions, use `class Mode(str, Enum)`.

## py-task-reference

severity: block  
concerns: correctness  
applies_when: Python code calls asyncio.create_task or ensure_future.

**Rule.** Never discard the result of `create_task`; keep a reference and surface its exceptions, preferably through a `TaskGroup`.

**Why.** The event loop holds only weak references, so an unreferenced task can be garbage-collected mid-run. A task that nobody awaits also loses its exception ("never retrieved").

**FAIL**

```python
asyncio.create_task(send_email(user))   # fire and forget
```

**PASS**

```python
async with asyncio.TaskGroup() as tg:
    tg.create_task(send_email(user))
# or: t = create_task(...); bg.add(t); t.add_done_callback(bg.discard)
```

**See also.** [py-cancellation](https://goodcodereview.com/pack-python.html#py-cancellation); for retried background work, [svc-idempotency-key](https://goodcodereview.com/service.md#svc-idempotency-key).

**Counterpoint.** "The set-and-discard recipe from the docs is enough." It keeps the task alive but still never retrieves the exception, and the docs point to TaskGroup for that. The same reference rule applies to tasks passed to `asyncio.shield`.

## py-test-hygiene

severity: discuss  
concerns: testing  
applies_when: A Python test uses the current date or time, or reaches the network.

**Rule.** Freeze time and block network in tests.

**Why.** Tests that depend on the clock or the network flake, and fail in CI at midnight or offline.

**FAIL**

```python
def test_token_expires():
    token = issue_token(ttl=60)
    assert token.expires_at > datetime.now()
```

**PASS**

```python
from freezegun import freeze_time

@freeze_time("2026-01-01 12:00:00")
def test_token_expires():
    token = issue_token(ttl=60)
    assert token.expires_at == datetime(2026, 1, 1, 12, 1)
# run with: pytest --disable-socket
```

**Counterpoint.** "Real time catches real bugs." It catches flakes. Test time-dependent logic with a frozen clock, and cover real-network behaviour in a separate, opt-in suite.