initial
This commit is contained in:
@@ -0,0 +1,150 @@
|
||||
---
|
||||
name: Clean code rules
|
||||
description: Best-practice standards for all production and test code
|
||||
applyTo: "**/*.py"
|
||||
---
|
||||
|
||||
# Clean code rules
|
||||
|
||||
These rules apply everywhere — every module, every function, every test. They are intentionally opinionated. If a rule conflicts with the architecture rules in `PRD.md` §17, the architecture rules win. If it conflicts with another best practice here, pick the one that produces the simpler, more readable result.
|
||||
|
||||
## Single responsibility
|
||||
|
||||
* One job per function. If a function name needs "and" to describe it, it's two functions.
|
||||
* One job per module. `parser.py` parses. `cache.py` caches. `formatting.py` formats. Don't mix.
|
||||
* One job per class. We rarely need classes outside Pydantic models, dataclasses, and the `HTTPClient`. Avoid OO for OO's sake.
|
||||
|
||||
## Function size
|
||||
|
||||
* Aim for under **30 lines** of body.
|
||||
* Past **50 lines** it's a code smell — extract helpers.
|
||||
* If you've got more than **3 levels of nesting**, the function wants splitting (extract the inner block into a helper named after what it does).
|
||||
|
||||
## Naming
|
||||
|
||||
* Names describe **intent**, not implementation. `get_or_fetch_ad`, not `process_ad`. `render_shortlist_markdown`, not `format2`.
|
||||
* Verbs for actions (`fetch_`, `parse_`, `score_`, `render_`).
|
||||
* Nouns for data (`FinnAd`, `EiendomUnit`, `shortlist`).
|
||||
* Boolean variables / parameters read as predicates: `force_refresh`, `include_eiendom_no`, `is_recently_sold`. Not `flag`, not `do_thing`.
|
||||
* Avoid abbreviations except those well-established in the domain (`url`, `ad`, `nok`, `bra`, `sqm`).
|
||||
* Norwegian terms stay Norwegian when they're domain vocabulary (`hybel`, `fellesgjeld`, `finnkode`). Don't translate `finnkode` to `finn_code` — it's a proper noun.
|
||||
|
||||
## Type hints
|
||||
|
||||
Required on every function signature, including private helpers. Mypy in strict mode is the goal.
|
||||
|
||||
```python
|
||||
# ❌
|
||||
def parse(html, base_url=None):
|
||||
...
|
||||
|
||||
# ✅
|
||||
def parse(html: str, base_url: str | None = None) -> FinnAd | None:
|
||||
...
|
||||
```
|
||||
|
||||
Use modern syntax: `X | None` over `Optional[X]`, `list[int]` over `List[int]`, `dict[str, Any]` over `Dict[str, Any]`.
|
||||
|
||||
## Comments
|
||||
|
||||
* Comments explain **WHY**, never **WHAT**. The code already says what.
|
||||
* If a comment is needed to explain *what* a line does, the line wants renaming or extracting.
|
||||
* Use docstrings for public functions, classes, and modules. One-line summary, blank line, optional details and examples.
|
||||
* No commented-out code. Delete it. Git remembers.
|
||||
* No `# TODO` without a date or issue reference. `# TODO(2026-05): replace once Eiendom.no confirms ...` is fine.
|
||||
|
||||
## DRY — Don't Repeat Yourself
|
||||
|
||||
If you write the same logic, regex, SQL, or format string **twice**, extract it. The decision table in `PRD.md` §17.2 tells you where it belongs.
|
||||
|
||||
The pre-merge anti-duplication checklist (from `PRD.md` §17.4):
|
||||
|
||||
1. Is this logic already implemented somewhere? (`grep` the function name and obvious keywords.)
|
||||
2. If I'm copy-pasting from another file, am I about to duplicate behavior that should live in one shared function?
|
||||
3. Can a new caller use an existing `service.py` function instead of writing its own orchestration?
|
||||
4. Is the same Pydantic field defined in two models? Factor out a base model.
|
||||
5. Am I formatting output in two places (CLI + MCP)? Move it to `formatting.py`.
|
||||
6. Am I opening a SQLite connection outside `cache.py`? Move it.
|
||||
7. Am I building an httpx call outside `http.py`? Move it.
|
||||
8. Am I writing a Norwegian-number / area / finnkode regex outside `parser.py`? Move it.
|
||||
9. Am I adding an env-var lookup outside `config.py`? Move it.
|
||||
10. Did I add a new behavior with only one front end (MCP or CLI)? If it should exist in both, the service function is missing.
|
||||
|
||||
A small amount of duplication is acceptable to keep boundaries clean — see `PRD.md` §17.8. Past a handful of lines, extract.
|
||||
|
||||
## Errors
|
||||
|
||||
* **Fail loudly** with actionable messages.
|
||||
|
||||
```python
|
||||
# ❌
|
||||
raise ValueError("bad input")
|
||||
|
||||
# ✅
|
||||
raise ValueError(f"Unknown listing_status {status!r}; expected one of {VALID_LISTING_STATUSES}")
|
||||
```
|
||||
|
||||
* **No silent failures.** `except Exception: pass` is forbidden. Catch the specific exception, log it, and either recover or re-raise.
|
||||
|
||||
* **Service raises; MCP wraps.** Service functions raise normal exceptions. The MCP tool boundary translates them into `{"error": True, "code": ..., "message": ...}`. CLI lets typer handle non-zero exits.
|
||||
|
||||
* **Graceful degradation is explicit.** If Eiendom.no enrichment fails, return a result with `eiendom_unit=None` and a warning, not a silently-missing field.
|
||||
|
||||
## State
|
||||
|
||||
* No global mutable state. The only module-level constants allowed are configuration values loaded from env in `config.py`.
|
||||
* No module-level caches (dicts, lists) that mutate. Use `cache.py` if you need persistence.
|
||||
* Pass dependencies in (HTTP clients, DB connections) for testability.
|
||||
|
||||
## Dead code
|
||||
|
||||
* No commented-out code.
|
||||
* No unused imports (ruff catches these — fix them, don't add `# noqa`).
|
||||
* No unused parameters (use `_` or remove).
|
||||
* No `if False:` blocks "for later".
|
||||
* Functions and classes that aren't called anywhere — delete them. Git keeps history.
|
||||
|
||||
## Magic numbers and strings
|
||||
|
||||
Anything that influences behavior and isn't self-explanatory belongs in `config.py` (env-controlled) or as a named module-level constant near the top of the file.
|
||||
|
||||
```python
|
||||
# ❌
|
||||
if days > 90:
|
||||
confidence = "low"
|
||||
|
||||
# ✅
|
||||
COMPS_STALE_AFTER_DAYS = 90
|
||||
|
||||
if days > COMPS_STALE_AFTER_DAYS:
|
||||
confidence = "low"
|
||||
```
|
||||
|
||||
URLs, timeouts, retries, TTLs, status codes — never inline.
|
||||
|
||||
## Imports
|
||||
|
||||
* Standard library first, third-party second, local last, separated by blank lines.
|
||||
* Ruff's `I` rules sort and group these — run `ruff check . --fix`.
|
||||
* No wildcard imports.
|
||||
* No relative imports above one level (`from ..thing import x` is a smell; refactor).
|
||||
* Each module's allowed import set is enforced by `tests/test_architecture.py`.
|
||||
|
||||
## Tests are first-class code
|
||||
|
||||
Same rules. Same type hints. Same naming. Same DRY. If a fixture is used in three test files, it goes in `conftest.py`. If three tests share a setup, factor it into a fixture.
|
||||
|
||||
## Reviewing your own change before commit
|
||||
|
||||
A 60-second self-review:
|
||||
|
||||
1. Did I add a function that already exists somewhere? (`grep` it.)
|
||||
2. Did I bypass `service.py`, `http.py`, `cache.py`, or `formatting.py`?
|
||||
3. Is everything typed?
|
||||
4. Did I leave a `print()`, `breakpoint()`, or commented-out block behind?
|
||||
5. Does the test for this change actually fail without the change?
|
||||
6. Did I update `PRD.md` or the relevant instruction file if I changed an architectural rule?
|
||||
|
||||
## When in doubt about a library API
|
||||
|
||||
Use the `context7` MCP server instead of guessing. See `docs.instructions.md`. Training-data memory of `pydantic.field_validator`, `typer.Option`, `mcp.tool` annotations, or `httpx.AsyncClient` is unreliable — they all change between versions.
|
||||
Reference in New Issue
Block a user