425 lines
22 KiB
Markdown
425 lines
22 KiB
Markdown
# AI-Optimized Python Style Guide
|
|
|
|
This document defines the Python style conventions for the Manual Slop codebase.
|
|
These deviate from PEP 8 / Google style to minimize token consumption when code
|
|
is processed by AI agents, while preserving readability for human review.
|
|
|
|
## 1. Indentation and Whitespace
|
|
|
|
- **Indentation:** 1 space per level. No tabs.
|
|
- **Continuation lines:** 1 space relative to the opening construct.
|
|
- **Blank lines:** Zero blank lines between function/method definitions within a class. One blank line between top-level definitions only when separating logically distinct sections.
|
|
- **Trailing whitespace:** None.
|
|
- **Rationale:** 1-space indentation reduces token count by ~40% compared to 4-space on deeply nested GUI code, with no loss of structural clarity for AST-based tools.
|
|
|
|
### Maximum Nesting Depth
|
|
|
|
- **Hard limit: 5 levels maximum.**
|
|
- AI agents consistently misinterpret Python scope via indentation. This hard limit prevents deeply nested blocks that confuse model interpretation.
|
|
- Classes and async handlers typically consume 1-2 levels before business logic begins, so 5 accommodates real-world usage patterns.
|
|
- **Enforcement:** Use ruff `[tool.ruff.lint.mccabe] max-complexity = 5` in the project linter config.
|
|
- **Refactoring mandate:** Any block exceeding 5 levels must be extracted into a named function. Do not "work around" this with tricks—extract the logic.
|
|
- **Rationale:** GUI callbacks and async handlers naturally want to nest. This constraint forces extraction of deep logic into testable, named units.
|
|
|
|
## 2. Type Annotations
|
|
|
|
- **All functions and methods** must have return type annotations.
|
|
- **All parameters** (except `self`/`cls`) must have type annotations.
|
|
- **Module-level and class-level variables** must have type annotations.
|
|
- **Use modern syntax:** `list[str]`, `dict[str, Any]`, `X | None` over `Optional[X]` where Python 3.10+ is available. Use `from __future__ import annotations` if needed.
|
|
- **Callable:** Use bare `Callable` for callback factories. Use `Callable[[ArgTypes], ReturnType]` when the signature is known and stable.
|
|
- **DearPyGui / ImGui callbacks:** Use `sender: Any, app_data: Any` for framework callbacks where the types are runtime-determined.
|
|
|
|
## 3. Imports
|
|
|
|
- Use `from __future__ import annotations` at the top of every module.
|
|
- Group imports: stdlib, third-party, local — separated by a blank line.
|
|
- Use `from typing import Any, Optional, Callable` etc. for type-only imports.
|
|
- Prefer `from x import Y` for specific symbols over `import x` when only one or two names are used.
|
|
|
|
## 4. Naming
|
|
|
|
- **snake_case** for modules, functions, methods, variables.
|
|
- **PascalCase** for classes.
|
|
- **ALL_CAPS** for module-level constants.
|
|
- **Single leading underscore** (`_name`) for internal/private members.
|
|
|
|
## 5. Docstrings
|
|
|
|
- Required on classes and non-trivial public functions.
|
|
- Use `"""triple double quotes"""`.
|
|
- One-line summary is sufficient for simple methods.
|
|
- Omit docstrings on obvious internal methods (e.g., `_cb_*` callbacks, `_render_*` UI methods) where the name is self-documenting.
|
|
|
|
## 6. String Formatting
|
|
- Prefer f-strings.
|
|
- Use double quotes (`"`) for strings by default.
|
|
- Use single quotes when the string contains double quotes.
|
|
|
|
## 7. Error Handling
|
|
- Never use bare `except:`.
|
|
- Use specific exception types.
|
|
- Prefer `if x is None:` over `if not x:` when testing for None specifically.
|
|
|
|
## 8. AI-Agent Specific Conventions
|
|
|
|
- **No redundant comments.** Do not add comments that restate what the code does. Only comment on *why* when non-obvious.
|
|
- **No empty `__init__.py` files.**
|
|
- **Minimal blank lines.** Token-efficient density is preferred over visual padding.
|
|
- **Short variable names are acceptable** in tight scopes (loop vars, lambdas). Use descriptive names for module-level and class attributes.
|
|
- **No diagnostic noise in production code (Added 2026-06-09).** `sys.stderr.write(f"[XYZ_DIAG] ...")` lines added to `src/*.py` for one-time debugging are technical debt the moment they ship. The project's production code should not contain `[XYZ_DIAG]` markers, `print(...debug...)` calls, or any other ad-hoc debug instrumentation. The right place for diagnostic output during a one-time investigation is `tests/artifacts/<test_name>.diag.log` (a log file) or a standalone `/tmp/diag_<name>.py` script. If you must instrument a production function for a single test run, the diag lines are part of the same atomic commit as the fix — they do not live uncommitted in the working tree. If you "revert everything," that means the diag lines are also reverted.
|
|
- **Test files ARE allowed to be diagnostic.** `tests/test_*.py` may use `print(..., file=sys.stderr)` freely for test output. The rule against diagnostic noise applies to `src/*.py` only.
|
|
|
|
## 10. Anti-OOP Conventions
|
|
|
|
### Philosophy
|
|
|
|
AI agents consistently misinterpret class hierarchies, method resolution, and inheritance. Flat function-call graphs are deterministic and traceable. OOP introduces scoping complexity that compounds with indentation.
|
|
|
|
### Hard Rules (Enforced by lint)
|
|
|
|
- **Never write a class for a single method.** Use a function.
|
|
- **Never use inheritance for code reuse.** Compose with standalone functions.
|
|
- **Never use private methods (`_method`).** Module-level functions with clear names suffice.
|
|
- **No nested classes.** Define helper types at module level.
|
|
- **No decorator classes.** Use plain functions with decorators.
|
|
|
|
### Class Justification Required
|
|
|
|
Every class definition MUST include a comment explaining WHY it is a class and not a function group or struct:
|
|
|
|
```python
|
|
# JUSTIFIED: Holds mutable shared state across multiple async operations
|
|
# Cannot be replaced by functions without passing state through every call
|
|
class AsyncOperationScheduler:
|
|
...
|
|
|
|
# NOT JUSTIFIED: Simply groups related functions
|
|
# Should be module-level functions in operations.py
|
|
class OperationHelper:
|
|
def validate(self): ...
|
|
def execute(self): ...
|
|
```
|
|
|
|
### Acceptability Criteria
|
|
|
|
A class is justified ONLY when ALL of:
|
|
|
|
1. It holds mutable state that must be encapsulated
|
|
2. It has 3+ related methods that share state
|
|
3. It implements a behavioral interface used polymorphically (not just data grouping)
|
|
|
|
### Refactoring Existing Classes (Strangler Fig Pattern)
|
|
|
|
When refactoring a class to functions:
|
|
|
|
1. Write test validating current behavior (prevents regression)
|
|
2. Extract one method at a time into module-level functions
|
|
3. Create wrapper function that delegates to class until migration complete
|
|
4. Delete class only when ALL callers migrated
|
|
5. Commit with `refactor(oop):` prefix
|
|
|
|
### Data Structures
|
|
|
|
- **Data-only containers:** Use `NamedTuple`, `dataclass(frozen=True)`, or plain `dict` — NOT classes
|
|
- **State machines:** Use dict-based transitions, not class + inheritance
|
|
- **Configuration:** Plain dict or `TypedDict`, not classes with defaults
|
|
|
|
### Anti-Patterns (Flagged by Ruff PLR rules)
|
|
|
|
- `PLR0912`: Too many branches — extract to functions
|
|
- `PLR6301`: No public methods — class is a namespace anti-pattern
|
|
- `PLR0206`: Descriptors in class body — use simple attributes
|
|
|
|
### Enforcement
|
|
|
|
```toml
|
|
[tool.ruff.lint.select]
|
|
select = ["E", "F", "W", "C90", "C4", "PLR0912", "PLR6301", "PLR0206"]
|
|
|
|
[tool.ruff.lint.plr]
|
|
max-returns = 4
|
|
max-locals = 8
|
|
max-args = 5
|
|
```
|
|
|
|
### 11. ImGui Defer Patterns
|
|
|
|
To prevent `PopID` or `End` leaks in immediate-mode rendering, and to keep code flat (0-1 levels of nesting) for AI agents, use the following patterns:
|
|
|
|
- **The Context Manager Pattern (Mandatory for complex blocks):**
|
|
Wrap all `Begin/End` blocks in `imscope` context managers (from `src/imgui_scopes.py`).
|
|
|
|
```python
|
|
with imscope.window("My Window") as (exp, opened):
|
|
if exp:
|
|
imgui.text("Hello")
|
|
|
|
with imscope.tab_item("My Tab") as (exp, _):
|
|
if exp:
|
|
self._render_tab_content()
|
|
```
|
|
|
|
This adds only 1 space of indentation (project standard) and guarantees the corresponding `End` is called even on early returns or exceptions. **Crucial:** Always check the `exp` (expanded/visible) state before rendering content to avoid ID conflicts and performance overhead.
|
|
|
|
- **The Flat Dispatch Pattern (Recommended for the main loop):**
|
|
|
|
To avoid nesting multiple window checks, use a dispatch helper that encapsulates the state check and the scope.
|
|
|
|
```python
|
|
self._render_window_if_open("My Window", self._render_my_panel)
|
|
```
|
|
|
|
This keeps the main GUI loop as a flat sequence of declarative calls.
|
|
|
|
## 12. Structural Dependency Mapping (SDM)
|
|
|
|
To assist AI agents in evaluating refactoring impact across dynamic codebases, all major definitions SHOULD include terse SDM tags at the end of their docstrings.
|
|
|
|
- **Format:** Tags are enclosed in square brackets at the end of the docstring body.
|
|
- **For Functions/Methods:** `[C: CallerA, CallerB]` — List of primary internal callers within the codebase.
|
|
- **For State Variables:**
|
|
- `[M: File:Line, Method]` — List of primary mutation points (where the value is assigned).
|
|
- `[U: File]` — Major codepaths of use (where the value is read but not changed).
|
|
|
|
## 13. Extreme Vertical Compaction & Alignment
|
|
|
|
To minimize token usage and enhance visual scanning for human reviewers, heavily compact repetitive logic, especially in GUI definitions:
|
|
|
|
- **Single-Line Conditionals:** Prefer `if cond: do_this()` over multiline blocks for simple assignments or function calls. **Note:** Function and method definition signatures (`def ...:`) must ALWAYS remain on their own isolated lines and should never be compacted.
|
|
- **Semicolon Stacking:** Chain closely related framework calls on a single line using semicolons (e.g., `imgui.same_line(); imgui.text("Label")`).
|
|
- **Alignment:** Align assignments and inline comments vertically when declaring batches of related variables or conditionals.
|
|
|
|
```python
|
|
if status == 'running': col = (0.0, 1.0, 0.0, 1.0)
|
|
elif status == 'starting': col = (1.0, 1.0, 0.0, 1.0)
|
|
elif status == 'error': col = (1.0, 0.0, 0.0, 1.0)
|
|
```
|
|
|
|
## 14. Logical Region Blocks
|
|
|
|
For files where many related methods/properties live in a single class (e.g., the `App` class in `src/gui_2.py` holding global UI state; the `src/ai_client.py` module holding 8 vendor entry points and supporting machinery), use `#region: Section Name` and `#endregion: Section Name` tags (or `# --- Section Name ---` for visual grouping) to strictly organize methods and state properties. This establishes a predictable structure that MCP tools and agents can leverage for contextual masking.
|
|
|
|
**Removed anti-pattern (2026-06-11):** the prior version of this section said "extremely large files that violate the Anti-OOP rule by necessity." That framing was wrong. Files are not "large" in any absolute sense; production codebases (Unreal, OS kernels, game engines) routinely have 10K+ line files. The "Anti-OOP" rule is about data-vs-behavior separation, not file size. The `App` class in `src/gui_2.py` is not "violating" anything by being large; it's the natural shape of a class that owns the GUI orchestration. The `#region` convention is for navigability, not as a workaround for "files that got too big."
|
|
|
|
**Hard rule on new `src/<thing>.py` files (added 2026-06-11):** New namespaced `src/<thing>.py` files may only be created on the user's explicit request. If you find yourself about to create one, ASK FIRST — don't just create it. Rationale: the user is the only one who can authorize a new top-level namespace. Defaults: helpers and sub-systems go in the parent module. E.g., AI-client-specific helpers go in `src/ai_client.py`; app-controller helpers go in `src/app_controller.py`; MCP-client helpers go in `src/mcp_client.py`. Even if the parent file is already 3K+ lines, the helper still goes there. If a new top-level `src/<thing>.py` is genuinely warranted (e.g., a truly new system that doesn't fit any existing parent), propose it in the next checkpoint or status note and wait for the user's explicit "yes, create it." See `AGENTS.md` "File Size and Naming Convention" for the full rule.
|
|
|
|
## 15. Modular Controller Pattern
|
|
|
|
To prevent "God Object" bloat in core controllers (like `AppController`):
|
|
|
|
- **Extract Logic:** Move all state-independent or purely utility logic to module-level functions.
|
|
- **Dependency Injection:** Module-level functions that require class state should accept the instance as their first argument (e.g., `def my_extracted_logic(controller: 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.
|
|
|
|
## 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
|
|
|
|
### 17.9 Banned: Local imports + aliasing-for-naming-convenience + repeated `from_dict()` (Added 2026-06-27)
|
|
|
|
**LLMs default to local imports with `as _PREFIX` aliasing.** This is the "I don't want to repeat the long name" pattern. It's banned. Local imports add overhead; aliasing hides intent; repeated `.from_dict()` calls in the same expression are wasteful.
|
|
|
|
**17.9a — Banned: Local imports inside functions**
|
|
|
|
```python
|
|
# BANNED:
|
|
def calculate_total(app):
|
|
from src.type_aliases import MMAUsageStats as _MMA # ← local import; defeats static analysis
|
|
return sum(_MMA.from_dict(u).model for u in app.mma_tier_usage.values())
|
|
|
|
# CORRECT:
|
|
# Add the import at the top of the module:
|
|
# from src.type_aliases import MMAUsageStats
|
|
|
|
def calculate_total(app):
|
|
return sum(u.model for u in app.mma_tier_usage.values())
|
|
```
|
|
|
|
**Why:** local imports:
|
|
- Add per-call import overhead (cached after first call, but still pollutes the namespace).
|
|
- Defeat static analysis (ruff/mypy can't see what's imported where).
|
|
- Hide dependencies (a reader has to scroll to find what's actually used).
|
|
- Encourage the aliasing anti-pattern (see 17.9b).
|
|
|
|
The ONLY exception: local imports inside `try/except ImportError` blocks for optional dependencies. Even then, prefer lazy module-level imports (`_module = None` then `global _module; _module = importlib.import_module(...)`).
|
|
|
|
**17.9b — Banned: `import X as _X` aliasing-for-naming-convenience**
|
|
|
|
```python
|
|
# BANNED:
|
|
from src.type_aliases import MMAUsageStats as _MMA
|
|
from src.openai_schemas import ToolCall as _TC
|
|
from src.models import FileItem as _FI
|
|
|
|
# CORRECT:
|
|
from src.type_aliases import MMAUsageStats
|
|
from src.openai_schemas import ToolCall
|
|
from src.models import FileItem
|
|
```
|
|
|
|
**Why:** `_PREFIX` aliasing is "I don't want to repeat the long name, so I'll shorten it." But the long name IS the documentation — `MMAUsageStats` tells you what it is; `_MMA` is opaque. The "long name" is rarely actually long enough to justify aliasing. If you find yourself aliasing to shorten, the real problem is the function is too long — extract.
|
|
|
|
**17.9c — Banned: Repeated `.from_dict()` calls in the same expression**
|
|
|
|
```python
|
|
# BANNED:
|
|
from src.type_aliases import MMAUsageStats as _MMA
|
|
total_cost = sum(cost_tracker.estimate_cost(
|
|
_MMA.from_dict(u).model or 'unknown',
|
|
_MMA.from_dict(u).input,
|
|
_MMA.from_dict(u).output,
|
|
) for u in app.mma_tier_usage.values())
|
|
|
|
# CORRECT:
|
|
total_cost = sum(cost_tracker.estimate_cost(
|
|
stats.model or 'unknown',
|
|
stats.input,
|
|
stats.output,
|
|
) for stats in (
|
|
MMAUsageStats.from_dict(u) if isinstance(u, dict) else u
|
|
for u in app.mma_tier_usage.values()
|
|
))
|
|
```
|
|
|
|
**Why:** repeated `.from_dict()` calls:
|
|
- Waste work (parse the same dict multiple times).
|
|
- Indicate a broken design (the variable's type isn't right).
|
|
- Should be cached in a local variable OR the type should be promoted at the boundary so `from_dict()` isn't called at the consumer site at all.
|
|
|
|
The CORRECT pattern (preferred): promote the type at the boundary. After `cruft_elimination_20260627`, `app.mma_tier_usage` is typed `dict[str, MMAUsageStats]` (the boundary does `from_dict()` ONCE). The consumer iterates `stats.model`, `stats.input`, `stats.output` directly. No `from_dict()` at the consumer site.
|
|
|
|
### 17.10 Enforcement (LLM-default anti-patterns)
|
|
|
|
- Pre-commit: every commit MUST pass ruff with the project's configured lint set (`pyproject.toml [tool.ruff.lint]`).
|
|
- Tier 2 review: reject any commit that adds a local import or `_PREFIX` alias.
|
|
- The static analysis script `scripts/audit_imports.py` (planned) flags local imports outside `try/except ImportError` blocks.
|
|
|
|
## 18. See Also — Per-File Pattern Demonstrations
|
|
|
|
The following per-source-file guides show these conventions applied in real code:
|
|
|
|
- **[docs/guide_gui_2.md](../../../docs/guide_gui_2.md):** §"UI Delegation Pattern" — 90+ module-level `render_xxx(app)` functions in `src/gui_2.py`. Every panel uses the `App` instance as injected state, never `self`.
|
|
- **[docs/guide_app_controller.md](../../../docs/guide_app_controller.md):** §"Hook API Surface" — `_predefined_callbacks` and `_gettable_fields` as handler maps, not if/elif chains. §"AppState" — dataclass with all-typed fields, no methods.
|
|
- **[docs/guide_command_palette.md](../../../docs/guide_command_palette.md):** §"CommandRegistry" — decorator-based registration (`@registry.register`) instead of list-of-dicts. §"defensive try/except wrapping" — handler-map style error isolation.
|
|
- **[docs/guide_mcp_client.md](../../../docs/guide_mcp_client.md):** §"3-Layer Security Model" — handler-map dispatch (`def dispatch(tool_name, ...)`) with one branch per tool, instead of polymorphism.
|
|
- **[docs/guide_multi_agent_conductor.md](../../../docs/guide_multi_agent_conductor.md):** §"WorkerPool" — module-level `run_mma_worker` function, not a class method. The `WorkerPool` is justified (encapsulates semaphore + executor + active set).
|
|
- **[docs/guide_models.md](../../../docs/guide_models.md):** §"Data Structures" — `dataclass(frozen=True)` and `pydantic` over OOP classes for data-only containers.
|