docs(review): Python adaptation files for clean-code, solid and code-review
The canonical Python stack has been pinned in ~/dev/.context/AGENT.md since local-dev v0.3.1, and three skills still had Go examples with no Python counterpart. Following the structure #18 settled on, and the existing solid/references/go-adaptation.md precedent, each gets a reference file rather than inline dilution of the primary doc. The stack table itself is NOT copied into any of them — each cites AGENT.md as its home. This repo spent today learning what a second copy of a fact costs. Content is the part a linter cannot give you: - clean-code: `raise X from e` as the analogue of `%w` error wrapping, dataclass over `dict[str, Any]`, and the two traps where the broken version reads tidier — mutable default arguments, and `if not amount:` for a value where zero is legitimate. - solid: `Protocol` over `ABC`, defined at the consumer, which is what makes DIP real in Python; plus what does NOT carry over from the Go rewrite (return structs/accept interfaces, embedding, package-as-deployment-unit). - code-review: a Python anti-pattern table parallel to the Go one, opening with the rule that a finding ruff or mypy already catches is not a review comment, it is a question about why the gate is not running. Refs #20 Bump-Type: minor
This commit is contained in:
@@ -281,5 +281,6 @@ During refactoring:
|
||||
|
||||
- `references/clean-code.md` — detailed naming rules, object calisthenics
|
||||
- `references/code-smells.md` — comprehensive smell catalog with examples
|
||||
- `references/python-adaptation.md` — Python conventions: error chaining, dataclasses, the truthiness trap
|
||||
- Load `solid` skill for SOLID principles with Go examples
|
||||
- Load `refactoring` skill for systematic refactoring techniques
|
||||
|
||||
@@ -0,0 +1,104 @@
|
||||
# Clean Code: Python Adaptation
|
||||
|
||||
The principles in `SKILL.md` are language-agnostic. This file covers the places where
|
||||
Python's idioms differ from the Go examples, and where following the Go shape produces
|
||||
bad Python.
|
||||
|
||||
The canonical stack (ruff, mypy --strict, pytest, uv, pydantic-settings, src-layout)
|
||||
lives in `~/dev/.context/AGENT.md` and is not restated here — one source of truth, and
|
||||
it is not this file.
|
||||
|
||||
## Reveal intent with types, not comments
|
||||
|
||||
`mypy --strict` is the Python analogue of Go's compiler, and it is non-negotiable for
|
||||
the same reason: a type is a claim the machine checks, and a comment is a claim nobody
|
||||
checks. Two consequences for clean code specifically:
|
||||
|
||||
- **`Any` is a hole in the argument.** Each one says "the reader is on their own here".
|
||||
If a value is genuinely polymorphic, say so precisely — `Protocol`, a union, a
|
||||
`TypeVar` — rather than surrendering.
|
||||
- **A `# type: ignore` needs the error code and a reason**: `# type: ignore[arg-type] #
|
||||
boto3 stubs lag the API`. A bare ignore is an unexplained exception to a rule the rest
|
||||
of the file follows.
|
||||
|
||||
## Errors: chain them, never swallow them
|
||||
|
||||
Go's `fmt.Errorf("operation: %w", err)` is `raise` with `from`:
|
||||
|
||||
```python
|
||||
# Bad — the original cause is gone, and so is the stack that produced it
|
||||
try:
|
||||
row = fetch(invoice_id)
|
||||
except KeyError:
|
||||
raise InvoiceNotFound(invoice_id)
|
||||
|
||||
# Good — the chain survives; the traceback shows both
|
||||
try:
|
||||
row = fetch(invoice_id)
|
||||
except KeyError as e:
|
||||
raise InvoiceNotFound(invoice_id) from e
|
||||
```
|
||||
|
||||
Two rules with no exceptions:
|
||||
|
||||
- **Never bare `except:`.** It catches `KeyboardInterrupt` and `SystemExit` too, so it
|
||||
turns Ctrl-C into a swallowed error. `except Exception` if you truly mean everything.
|
||||
- **Never a silent `pass`.** An exception handler that does nothing is a decision to
|
||||
ignore a failure; if that is genuinely right, the handler needs a comment saying why,
|
||||
and usually a log line. Most of the time it is a bug being stored for later.
|
||||
|
||||
## Data classes over dicts
|
||||
|
||||
A `dict[str, Any]` passed between functions is Go's `map[string]interface{}` — every
|
||||
reader has to reconstruct the shape from the code that writes it.
|
||||
|
||||
```python
|
||||
# Bad: shape known only by reading every producer
|
||||
def total(invoice: dict[str, Any]) -> int: ...
|
||||
|
||||
# Good: shape declared once, checked everywhere
|
||||
@dataclass(frozen=True, slots=True)
|
||||
class Invoice:
|
||||
id: InvoiceId
|
||||
lines: tuple[Line, ...]
|
||||
currency: str # ISO 4217
|
||||
|
||||
def total(invoice: Invoice) -> Money: ...
|
||||
```
|
||||
|
||||
`frozen=True` where the value is conceptually immutable — it makes accidental mutation
|
||||
an error rather than a surprise at a distance. Use `pydantic` models at the *boundary*
|
||||
(env config, HTTP payloads) where validation is the point, and dataclasses inside,
|
||||
where validation has already happened.
|
||||
|
||||
## Two Python-specific traps that read as clean code
|
||||
|
||||
**Mutable default arguments** are evaluated once, at definition:
|
||||
|
||||
```python
|
||||
def add_line(line: Line, lines: list[Line] = []) -> list[Line]: # shared across calls
|
||||
```
|
||||
|
||||
Use `None` and build inside. Ruff catches this (`B006`); the point of naming it here is
|
||||
that the broken version looks tidier than the correct one.
|
||||
|
||||
**Truthiness is not presence.** `if not amount:` is true for `0`, `0.0`, `""`, `[]` and
|
||||
`None` alike. For anything where zero is a legitimate value — money, counts, offsets —
|
||||
write `if amount is None:`. This is the single most common way a Python function
|
||||
silently mishandles a valid input.
|
||||
|
||||
## Structure
|
||||
|
||||
- `src/` layout, `pyproject.toml` only. A flat layout lets tests import the working
|
||||
directory instead of the installed package, so the suite passes on a package that
|
||||
cannot actually be installed.
|
||||
- Module names are the unit of cohesion, as Go packages are. `utils.py` is the Python
|
||||
spelling of a god package: it has no reason to change because it has no subject.
|
||||
- Prefer functions to classes with one method. A class whose only job is to hold
|
||||
constructor arguments and expose `run()` is a closure with ceremony.
|
||||
|
||||
## See also
|
||||
|
||||
- `references/clean-code.md` — the language-agnostic practices
|
||||
- `references/code-smells.md` — the smell catalogue, all of which apply here
|
||||
- `tdd/references/python.md` — the test-side conventions
|
||||
@@ -279,6 +279,8 @@ treats `pass` as success, `fail` as failure, `skip` as neither.
|
||||
|
||||
## References
|
||||
|
||||
- `references/python-adaptation.md` — the Python anti-pattern table, and what ruff/mypy
|
||||
should have caught before review
|
||||
- `clean-code/references/code-smells.md` — comprehensive smell catalog
|
||||
- Load `refactoring` skill when recommending fixes
|
||||
- Load `test-design` skill for reviewing test quality
|
||||
|
||||
@@ -0,0 +1,60 @@
|
||||
# Code Review: Python Anti-Patterns
|
||||
|
||||
`SKILL.md` carries the Go anti-pattern table. This is the Python one. The review
|
||||
process, severity classification and output format above it are language-neutral and
|
||||
apply unchanged.
|
||||
|
||||
Canonical stack in `~/dev/.context/AGENT.md`. Two gates decide most of this file:
|
||||
`ruff` and `mypy --strict`. **If a finding is something ruff or mypy already catches,
|
||||
the review comment is not "fix this line" — it is "why is the gate not running?"**
|
||||
|
||||
## The table
|
||||
|
||||
| Anti-pattern | What to look for | Why it matters |
|
||||
|---|---|---|
|
||||
| Bare `except:` | `except:` with no exception class | Swallows `KeyboardInterrupt` and `SystemExit`; Ctrl-C stops working |
|
||||
| Silent `pass` | `except X: pass` with no comment | A decision to ignore a failure, recorded nowhere |
|
||||
| Lost cause | `raise Y(...)` inside `except X as e` with no `from e` | Traceback loses the original failure — the Python analogue of a naked `fmt.Errorf` |
|
||||
| Mutable default | `def f(x, acc=[])` or `={}` | Evaluated once at definition; state leaks between calls |
|
||||
| Truthiness for presence | `if not amount:` where `0` is valid | Silently mishandles zero, empty string, empty list |
|
||||
| `Any` as an escape | `Any` in a signature that could be `Protocol`/union/`TypeVar` | Removes exactly the checking `--strict` was adopted for |
|
||||
| Unexplained ignore | bare `# type: ignore` | No error code, no reason; permanent by default |
|
||||
| `assert` in production code | `assert user.is_admin` outside tests | `python -O` strips asserts — the check vanishes in a tuned deployment |
|
||||
| Import-time side effects | connections, env reads, or I/O at module scope | Cannot be injected, cannot be tested, ordering-dependent |
|
||||
| Sync I/O in `async def` | `requests.get`, `time.sleep`, blocking file reads | Blocks the whole event loop; looks concurrent, is not |
|
||||
| Un-awaited coroutine | a call to an `async def` with no `await` | Silently does nothing; "coroutine was never awaited" is a warning, not an error |
|
||||
| `float` for money | `float` amounts, `round()` on currency | `0.1 + 0.2 != 0.3`; use `Decimal` or int minor units |
|
||||
| Broad `Exception` re-raise | `except Exception` around a whole function body | Turns a specific failure into an unclassifiable one |
|
||||
| Mutating a shared default/global | module-level `list`/`dict` mutated by functions | The Python spelling of shared mutable state |
|
||||
| `dict[str, Any]` as a domain type | dicts passed between layers | Shape known only by reading every producer; use a dataclass |
|
||||
| Star imports | `from x import *` | Breaks `--strict`, hides shadowing, defeats grep |
|
||||
| `os.path` string surgery | manual path joining, `+ "/"` | `pathlib` exists and is correct on every platform |
|
||||
| `subprocess(..., shell=True)` with interpolation | f-strings into a shell command | Injection; pass a list, no shell |
|
||||
|
||||
## What to check that a linter cannot
|
||||
|
||||
- **Does the test suite assert the failure mode, or only the happy path?** `pytest.raises`
|
||||
with no `match=` accepts any message, including one from an unrelated bug.
|
||||
- **Are fixtures doing real work?** A `conftest.py` fixture with a `session` scope and
|
||||
mutable state couples every test that touches it; failures then depend on ordering.
|
||||
- **Is the mock mocking a boundary or a collaborator you own?** The latter tests the mock.
|
||||
- **Does the type say what the code means?** `-> dict` and `-> tuple[int, int]` both pass
|
||||
`--strict` and both tell the reader nothing. A named type is a review comment worth
|
||||
making.
|
||||
- **Would `--strict` actually run on this file?** Check for a per-module override in
|
||||
`pyproject.toml` silently exempting it. An exemption nobody revisits is a hole with a
|
||||
changelog entry.
|
||||
|
||||
## Severity in this context
|
||||
|
||||
- **Blocking:** anything in the table above under security or correctness — `shell=True`
|
||||
interpolation, `assert` as an authorization check, `float` money, sync I/O in async.
|
||||
- **Should fix:** typing escapes (`Any`, unexplained ignores), mutable defaults,
|
||||
truthiness-for-presence.
|
||||
- **Consider:** structural comments — dataclass instead of dict, narrower Protocol.
|
||||
|
||||
## See also
|
||||
|
||||
- `clean-code/references/python-adaptation.md` — the style-level conventions
|
||||
- `solid/references/python-adaptation.md` — Protocol over ABC, DIP in Python
|
||||
- `tdd/references/python.md` — the test-side conventions this review checks against
|
||||
@@ -258,5 +258,6 @@ Domain types know nothing about HTTP, SQL, or external APIs.
|
||||
|
||||
- `references/solid-principles.md` — canonical SOLID reference with TypeScript examples
|
||||
- `references/go-adaptation.md` — this workspace's Go-specific rewrite of each principle
|
||||
- `references/python-adaptation.md` — the Python rewrite: Protocol over ABC, and what does not carry over from Go
|
||||
- Load `clean-code` skill for naming and structure
|
||||
- Load `code-review` skill for detecting violations during review
|
||||
|
||||
@@ -0,0 +1,104 @@
|
||||
# SOLID Principles: Python Adaptation
|
||||
|
||||
`references/go-adaptation.md` rewrites each principle for Go. This file does the same
|
||||
for Python, and marks the places where the Go rewrite's advice does *not* carry over.
|
||||
|
||||
The canonical Python stack lives in `~/dev/.context/AGENT.md`; it is not restated here.
|
||||
|
||||
## The one idea that changes everything: `Protocol`
|
||||
|
||||
Go's interfaces are structural — a type satisfies one by having the methods, with no
|
||||
declaration. Python has the same thing since 3.8, and it is the right default:
|
||||
|
||||
```python
|
||||
from typing import Protocol
|
||||
|
||||
class InvoiceStore(Protocol):
|
||||
def get(self, invoice_id: InvoiceId) -> Invoice: ...
|
||||
def save(self, invoice: Invoice) -> None: ...
|
||||
```
|
||||
|
||||
- **Define the Protocol where it is *consumed*, not where it is implemented.** This is
|
||||
Go's "accept interfaces, return structs" and it is what makes DIP real: the high-level
|
||||
module owns the abstraction, and the adapter depends on it rather than the reverse.
|
||||
- **Prefer `Protocol` to `ABC`.** An ABC forces implementers to inherit, which couples
|
||||
them to your package and makes a third-party class impossible to adapt without a
|
||||
wrapper. Use an ABC only when you genuinely need shared implementation, not just a
|
||||
contract.
|
||||
- Add `@runtime_checkable` only if you actually call `isinstance` — and if you are
|
||||
calling `isinstance` against a Protocol, check whether the real problem is a missing
|
||||
polymorphic method (an LSP smell).
|
||||
|
||||
## S — Single Responsibility
|
||||
|
||||
Same as Go: the unit is the module, not just the class. `utils.py` is the Python god
|
||||
package — it cannot have one reason to change because it has no subject.
|
||||
|
||||
Python-specific tell: a class whose `__init__` stores arguments and whose only other
|
||||
method is `run()` or `execute()`. That is a function with ceremony. Write the function.
|
||||
|
||||
## O — Open/Closed
|
||||
|
||||
Extension in Python usually means **passing behaviour in**, not subclassing:
|
||||
|
||||
```python
|
||||
# Closed for modification: adding a rule does not touch this function
|
||||
def apply_rules(invoice: Invoice, rules: Sequence[Rule]) -> Invoice: ...
|
||||
```
|
||||
|
||||
`functools.singledispatch` is the idiomatic open extension point when dispatch is on
|
||||
type and the set of types grows outside your module. Beware the alternative that looks
|
||||
Pythonic and is not: monkey-patching is modification pretending to be extension, and it
|
||||
is invisible at the call site.
|
||||
|
||||
## L — Liskov
|
||||
|
||||
The Python-specific violation is **signature widening/narrowing that the type checker
|
||||
would catch if you let it**. Under `mypy --strict`, an override that narrows a parameter
|
||||
type or widens a return type is an error — which means most LSP violations in Python are
|
||||
type errors you chose not to see. This is the strongest argument for `--strict` in a
|
||||
design document rather than a style one.
|
||||
|
||||
Raising a *new* exception type in an override is the violation `mypy` will not catch:
|
||||
callers wrote `except StoreError`, and your subclass raises `TimeoutError`. Exceptions
|
||||
are part of the contract; Python just will not check them for you.
|
||||
|
||||
## I — Interface Segregation
|
||||
|
||||
Protocols make this cheap: define the narrow one you need at the call site.
|
||||
|
||||
```python
|
||||
class InvoiceReader(Protocol):
|
||||
def get(self, invoice_id: InvoiceId) -> Invoice: ...
|
||||
```
|
||||
|
||||
A function that only reads should accept `InvoiceReader`, not the full `InvoiceStore`.
|
||||
The test double then implements one method instead of eight, which is the practical
|
||||
payoff and the reason this principle survives contact with real code.
|
||||
|
||||
## D — Dependency Inversion
|
||||
|
||||
Constructor injection, exactly as in Go. Two Python-specific hazards:
|
||||
|
||||
- **Import-time side effects are hidden dependencies.** A module that opens a database
|
||||
connection or reads an env var at import has a dependency nothing can inject and no
|
||||
test can replace. Configuration belongs in `pydantic-settings` objects passed in.
|
||||
- **Default arguments are compile-time bindings.** `def send(client: Client = Client())`
|
||||
constructs the default once, at definition, and welds it to every caller. Default to
|
||||
`None` and construct inside, or require it.
|
||||
|
||||
## What does not carry over from the Go rewrite
|
||||
|
||||
- **"Return structs, accept interfaces"** — the returning half is about Go's zero values
|
||||
and nil interfaces. In Python, returning a Protocol type is fine and often better.
|
||||
- **Small interfaces because of embedding** — Python has no embedding; composition is
|
||||
explicit delegation, which is more verbose but reads honestly.
|
||||
- **Package-level responsibility as the unit of deployment** — Python packaging is much
|
||||
looser. The module boundary is still the right unit for *reasoning*, but do not expect
|
||||
it to be enforced.
|
||||
|
||||
## See also
|
||||
|
||||
- `references/solid-principles.md` — the canonical, language-neutral reference
|
||||
- `references/go-adaptation.md` — the Go rewrite; contrast is instructive
|
||||
- `clean-code/references/python-adaptation.md` — style-level Python conventions
|
||||
Reference in New Issue
Block a user