docs(styleguide): add python.md §17 (Banned Patterns — LLM Default Anti-Patterns)
This commit is contained in:
@@ -213,7 +213,127 @@ To prevent "God Object" bloat in core controllers (like `AppController`):
|
||||
- **Handler Maps:** Replace massive `if/elif` blocks (like those in event dispatchers) with dictionaries mapping keys to module-level handler functions.
|
||||
- **Inner Class Extraction:** Never define nested classes or functions within methods. Move them to the module level.
|
||||
|
||||
## 16. See Also — Per-File Pattern Demonstrations
|
||||
## 17. Banned Patterns (LLM Default Anti-Patterns) (Added 2026-06-25)
|
||||
|
||||
**C11/Odin/Jai semantics in a Python runtime.** This codebase is written in Python because of practical constraints, but the convention is to make Python behave as close to a statically-typed value-typed language as the runtime allows. LLMs default to the following patterns because that's what idiomatic Python training data looks like. **All of these are BANNED in non-boundary code.** See `data_oriented_design.md` §8.5 for the canonical mandate.
|
||||
|
||||
### 17.1 Banned: `dict[str, Any]`
|
||||
|
||||
```python
|
||||
# BANNED:
|
||||
def process(event: dict[str, Any]) -> None:
|
||||
if event.get("kind") == "tool_call":
|
||||
|
||||
# BANNED:
|
||||
flat: dict[str, Any] = project_manager.flat_config(...)
|
||||
|
||||
# CORRECT:
|
||||
def process(event: CommsLogEntry) -> None:
|
||||
if event.kind == "tool_call":
|
||||
|
||||
# CORRECT (boundary only):
|
||||
def _parse_wire(raw: str) -> Metadata:
|
||||
return Metadata.from_dict(tomllib.loads(raw))
|
||||
```
|
||||
|
||||
### 17.2 Banned: `Any`
|
||||
|
||||
```python
|
||||
# BANNED:
|
||||
def _to_typed_tool_call(tc: Any) -> ToolCall:
|
||||
return ToolCall(id=getattr(tc, "id", "") or "", ...)
|
||||
|
||||
# CORRECT:
|
||||
def _parse_wire_tool_call(wire: dict[str, Any]) -> ToolCall:
|
||||
"""Boundary: parse MCP wire dict to typed ToolCall."""
|
||||
return ToolCall.from_dict(wire)
|
||||
```
|
||||
|
||||
### 17.3 Banned: `Optional[T]` returns
|
||||
|
||||
```python
|
||||
# BANNED:
|
||||
def find_ticket(self, id: str) -> Optional[Ticket]:
|
||||
for t in self.active_tickets:
|
||||
if t.id == id: return t
|
||||
return None # ← silent failure; consumer has to None-check
|
||||
|
||||
# CORRECT (Result pattern):
|
||||
def find_ticket(self, id: str) -> Result[Ticket]:
|
||||
for t in self.active_tickets:
|
||||
if t.id == id: return Result(data=t)
|
||||
return Result(data=NIL_TICKET, errors=[ErrorInfo(...)]) # drain point handles
|
||||
|
||||
# CORRECT (NIL_T sentinel — preferred when consumer just reads fields):
|
||||
def find_ticket(self, id: str) -> Ticket:
|
||||
for t in self.active_tickets:
|
||||
if t.id == id: return t
|
||||
return NIL_TICKET # zero-initialized frozen dataclass; safe to read fields
|
||||
```
|
||||
|
||||
### 17.4 Banned: `hasattr()` for entity type dispatch
|
||||
|
||||
```python
|
||||
# BANNED:
|
||||
def handle_event(self, event: Metadata) -> None:
|
||||
if hasattr(event, 'tool_calls'):
|
||||
# tool call path
|
||||
elif hasattr(event, 'source_tier'):
|
||||
# mma path
|
||||
elif hasattr(event, 'path'):
|
||||
# file path
|
||||
|
||||
# CORRECT (typed Union dispatch):
|
||||
def handle_event(self, event: CommsLogEntry | FileItem | HistoryMessage) -> None:
|
||||
if isinstance(event, CommsLogEntry):
|
||||
# mma path
|
||||
elif isinstance(event, FileItem):
|
||||
# file path
|
||||
elif isinstance(event, HistoryMessage):
|
||||
# tool call path
|
||||
|
||||
# CORRECT (preferred — refactor so no dispatch is needed):
|
||||
def _handle_comms_entry(self, event: CommsLogEntry) -> None: ...
|
||||
def _handle_file_item(self, event: FileItem) -> None: ...
|
||||
def _handle_history(self, event: HistoryMessage) -> None: ...
|
||||
```
|
||||
|
||||
### 17.5 Banned: `getattr(x, 'field', default)` for type dispatch
|
||||
|
||||
```python
|
||||
# BANNED:
|
||||
tool_id = getattr(tc, "id", "") or ""
|
||||
tool_name = getattr(tc.function, "name", "") or ""
|
||||
|
||||
# CORRECT:
|
||||
tool_id = tc.id
|
||||
tool_name = tc.function.name
|
||||
```
|
||||
|
||||
### 17.6 Banned: `.get('field', default)` on a `dict[str, Any]`
|
||||
|
||||
```python
|
||||
# BANNED:
|
||||
tier = entry.get('source_tier', 'main')
|
||||
model = entry.get('model', 'unknown')
|
||||
|
||||
# CORRECT (direct attribute access on the typed dataclass):
|
||||
tier = entry.source_tier
|
||||
model = entry.model
|
||||
```
|
||||
|
||||
### 17.7 The one exception: the boundary layer
|
||||
|
||||
The ONLY place these patterns are allowed is at the literal wire boundary — the function that calls `tomllib.load()`, `json.loads()`, or a vendor SDK's response parser. The boundary is 2-3 functions per file. Every consumer IMMEDIATELY converts to a typed dataclass via `from_dict()`.
|
||||
|
||||
### 17.8 Enforcement
|
||||
|
||||
- `scripts/audit_weak_types.py --strict` — flags `dict[str, Any]`, `Any`, anonymous tuple returns
|
||||
- `scripts/audit_optional_in_3_files.py --strict` — flags `Optional[T]` in the 3 refactored files (extended to ALL `src/*.py` per the c11_python track)
|
||||
- The new `boundary_layer` audit (planned in `conductor/tracks/cruft_elimination_20260627/spec.md`) — documents every `Metadata` usage with justification
|
||||
- Pre-commit: every commit MUST pass all three audits above
|
||||
|
||||
## 18. See Also — Per-File Pattern Demonstrations
|
||||
|
||||
The following per-source-file guides show these conventions applied in real code:
|
||||
|
||||
|
||||
Reference in New Issue
Block a user