feat(skills): code-standards architecture + tests sections, scattered-models check (#256)
* feat(skills): code-standards architecture + tests sections * feat(infra): scattered-models AST check in project-status * test(infra): scattered-models check tests (10 cases) --------- Co-authored-by: opencode-agent <agent@opencode.local>
This commit is contained in:
Sergey
GitHub
parent
aa1e5c08ae
commit
8e00969c3e
3 changed files with 385 additions and 0 deletions
|
|
@ -410,6 +410,129 @@ def _check_flat_layout(ptype: ProjectType, ctx: RepoCtx) -> CheckResult | None:
|
|||
)
|
||||
|
||||
|
||||
def _is_tortoise_model_base(base: ast.expr) -> bool:
|
||||
"""True if a class base references a Tortoise/ORM ``Model``.
|
||||
|
||||
Matches ``Model``, ``tortoise.Model``, ``models.Model``, ``tortoise.models.Model``.
|
||||
Does NOT match unrelated ``Model`` classes from other libraries — the caller
|
||||
is expected to have already verified the file imports ``tortoise`` or its
|
||||
own ``db.models`` module. Used by ``_check_scattered_models`` to detect
|
||||
ORM models living outside ``src/<package>/db/models/``.
|
||||
"""
|
||||
# bare ``Model`` (Name) — common when ``from tortoise import Model``
|
||||
if isinstance(base, ast.Name) and base.id == "Model":
|
||||
return True
|
||||
# ``tortoise.Model`` / ``models.Model`` / ``tortoise.models.Model`` (Attribute)
|
||||
if isinstance(base, ast.Attribute):
|
||||
# last attribute segment must be ``Model``; accept any qualifier
|
||||
# (``tortoise``, ``models``, ``db.models``, ...) — the import is
|
||||
# validated separately by the caller's import check.
|
||||
return base.attr == "Model"
|
||||
return False
|
||||
|
||||
|
||||
def _file_imports_tortoise_or_models(source: str, pkg: str) -> bool:
|
||||
"""True if the file imports ``tortoise`` or its own ``db.models`` module.
|
||||
|
||||
Used to suppress false positives in ``_check_scattered_models``: a class
|
||||
named ``Model`` from an unrelated library (e.g. ``pydantic.BaseModel`` is
|
||||
already excluded by name, but other libs may define their own ``Model``)
|
||||
should not trigger the WARN unless the file actually uses Tortoise/ORM.
|
||||
|
||||
The check is AST-based (walks ``ast.Import`` and ``ast.ImportFrom``) so it
|
||||
is robust to comments and strings mentioning those names.
|
||||
"""
|
||||
try:
|
||||
tree = ast.parse(source)
|
||||
except SyntaxError:
|
||||
return False
|
||||
models_module = f"{pkg}.db.models"
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, ast.Import):
|
||||
for alias in node.names:
|
||||
if alias.name == "tortoise" or alias.name.startswith("tortoise."):
|
||||
return True
|
||||
elif isinstance(node, ast.ImportFrom):
|
||||
module = node.module or ""
|
||||
if module == "tortoise" or module.startswith("tortoise."):
|
||||
return True
|
||||
if module == models_module or module.startswith(models_module + "."):
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _scan_file_for_models(py_file: Path, ctx: RepoCtx, pkg: str) -> list[CheckResult]:
|
||||
"""Scan a single ``.py`` file for ORM ``class X(Model)`` definitions.
|
||||
|
||||
Returns one ``WARN`` CheckResult per offending class (with the file's
|
||||
relative path). Returns ``[]`` if the file does not import tortoise or
|
||||
the package's own ``db.models`` module (false-positive guard), or if no
|
||||
class inherits from ``Model``.
|
||||
"""
|
||||
try:
|
||||
source = py_file.read_text(encoding="utf-8-sig")
|
||||
except OSError:
|
||||
return []
|
||||
if not _file_imports_tortoise_or_models(source, pkg):
|
||||
return []
|
||||
try:
|
||||
tree = ast.parse(source)
|
||||
except SyntaxError:
|
||||
return []
|
||||
results: list[CheckResult] = []
|
||||
rel = py_file.relative_to(ctx.root)
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.ClassDef):
|
||||
continue
|
||||
for base in node.bases:
|
||||
if _is_tortoise_model_base(base):
|
||||
results.append(
|
||||
CheckResult(
|
||||
CheckStatus.WARN,
|
||||
str(rel),
|
||||
f"class {node.name}(Model) вне db/models/ — "
|
||||
"models must live in src/<pkg>/db/models/",
|
||||
)
|
||||
)
|
||||
break
|
||||
return results
|
||||
|
||||
|
||||
def _check_scattered_models(ctx: RepoCtx, ptype: ProjectType) -> list[CheckResult]:
|
||||
"""AST-чек: ORM models должны жить в ``src/<package>/db/models/``.
|
||||
|
||||
Scans ``.py`` files in ``src/<package>/`` (excluding ``db/models/``) for
|
||||
``class X(Model)`` / ``class X(tortoise.Model)``. If found → WARN with the
|
||||
offending relative path. Models in ``db/models/`` are OK; files that do not
|
||||
import ``tortoise`` or the package's own ``db.models`` module are skipped
|
||||
(suppresses false positives from unrelated libraries defining ``Model``).
|
||||
|
||||
Skips for non-backend/fullstack types, flat layouts (no ``src/<package>/``),
|
||||
and repos without a resolvable package name. Returns ``[]`` in all skip
|
||||
cases so the caller appends nothing.
|
||||
"""
|
||||
if ptype not in {ProjectType.BACKEND, ProjectType.FULLSTACK}:
|
||||
return []
|
||||
pkg = _resolve_package_name(ctx)
|
||||
if pkg is None:
|
||||
return []
|
||||
# For FULLSTACK, the backend lives under ``backend/``; for BACKEND, at root.
|
||||
src_pkg = ctx.root / "src" / pkg
|
||||
if ptype == ProjectType.FULLSTACK:
|
||||
backend_src = ctx.root / "backend" / "src" / pkg
|
||||
if backend_src.is_dir():
|
||||
src_pkg = backend_src
|
||||
if not src_pkg.is_dir():
|
||||
return []
|
||||
models_dir = src_pkg / "db" / "models"
|
||||
results: list[CheckResult] = []
|
||||
for py_file in src_pkg.rglob("*.py"):
|
||||
if models_dir in py_file.parents or py_file == models_dir:
|
||||
continue
|
||||
results.extend(_scan_file_for_models(py_file, ctx, pkg))
|
||||
return results
|
||||
|
||||
|
||||
def _check_type_specific_structure(ptype: ProjectType, ctx: RepoCtx) -> list[CheckResult]:
|
||||
"""Type-specific extra checks beyond the expected dirs list."""
|
||||
results: list[CheckResult] = []
|
||||
|
|
@ -424,6 +547,7 @@ def _check_type_specific_structure(ptype: ProjectType, ctx: RepoCtx) -> list[Che
|
|||
flat = _check_flat_layout(ptype, ctx)
|
||||
if flat is not None:
|
||||
results.append(flat)
|
||||
results.extend(_check_scattered_models(ctx, ptype))
|
||||
return results
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -25,3 +25,113 @@ description: Universal code standards for any language. Use when writing, refact
|
|||
- AGENTS.md правило «No comments unless requested» — это **default**: код без комментариев
|
||||
- Этот skill описывает **исключение**: Google-style docstrings на английском для публичных API — когда контракт warrants (библиотечный API, public surface)
|
||||
- Описывай **зачем**, а не **что** — код и так говорит что делает
|
||||
|
||||
## 5. Architecture: good vs bad
|
||||
|
||||
Слои для **backend**: `routes → schemas → services → db/models` (4-tier, однонаправленный). Роуты тонкие (импортируют только `services` + `schemas`), сервисы работают с `db/models`, бизнес-логика здесь. `project-status.py` enforces subset (thin routes, centralized models); этот раздел объясняет «почему».
|
||||
|
||||
### Backend (подробно)
|
||||
|
||||
**GOOD tree (синтетический):**
|
||||
```
|
||||
src/<package>/
|
||||
├── api/
|
||||
│ ├── v1/
|
||||
│ │ ├── routes/users.py ← тонкие роуты, импортируют только services + schemas
|
||||
│ │ ├── dependencies.py ← Depends(), get_current_user
|
||||
│ │ └── router.py
|
||||
│ └── router.py
|
||||
├── config/
|
||||
│ ├── settings.py ← pydantic-settings
|
||||
│ └── logger.py ← loguru setup
|
||||
├── db/
|
||||
│ ├── connection.py ← Tortoise.init
|
||||
│ └── models/ ← ВСЕ ORM-модели здесь (centralized)
|
||||
│ ├── user.py
|
||||
│ ├── post.py
|
||||
│ └── comment.py
|
||||
├── schemas/ ← Pydantic DTO (НЕ Tortoise models)
|
||||
│ ├── base.py
|
||||
│ ├── user.py
|
||||
│ └── post.py
|
||||
├── services/ ← бизнес-логика (работает с db/models)
|
||||
│ ├── user_service.py
|
||||
│ └── post_service.py
|
||||
└── utils/
|
||||
└── metadata.py
|
||||
```
|
||||
|
||||
**Правила GOOD:**
|
||||
1. Все ORM-модели в `db/models/` (centralized)
|
||||
2. Schemas (Pydantic) отдельно от models (Tortoise) — НЕ смешивать
|
||||
3. Роуты тонкие — импортируют только `services` и `schemas`
|
||||
4. Сервисы работают с `db/models` — бизнес-логика здесь
|
||||
5. Слои: routes → schemas → services → db/models (4-tier, однонаправленный)
|
||||
|
||||
**BAD tree 1 — feature-scatter:**
|
||||
```
|
||||
src/<package>/
|
||||
├── channels/
|
||||
│ ├── models.py ← ❌ модель здесь (scatter)
|
||||
│ ├── routes.py
|
||||
│ └── service.py
|
||||
├── monitor/
|
||||
│ ├── models.py ← ❌ ещё модель здесь
|
||||
│ └── routes.py
|
||||
├── logs/
|
||||
│ └── models.py ← ❌ и здесь
|
||||
├── models.py ← ❌ root-level модель
|
||||
├── db.py ← ❌ connection flat (не db/connection.py)
|
||||
└── main.py
|
||||
```
|
||||
|
||||
Проблемы BAD 1: модели раскиданы по feature-папкам; `models.py` в root; `db.py` flat; не publishable; Tortoise `modules` должен перечислять 4+ файла вручную.
|
||||
|
||||
**BAD tree 2 — mixed-layers:**
|
||||
```
|
||||
src/<package>/
|
||||
├── api/
|
||||
│ ├── v1/
|
||||
│ │ └── users.py ← ❌ роут содержит бизнес-логику + Tortoise queries
|
||||
│ └── models.py ← ❌ модели в api/ (не в db/models/)
|
||||
├── services/
|
||||
│ └── user_service.py
|
||||
│ └── schemas.py ← ❌ schemas в services/ (не в schemas/)
|
||||
└── main.py
|
||||
```
|
||||
|
||||
Проблемы BAD 2: роут делает Tortoise queries напрямую (не тонкий); модели в `api/models.py` (не `db/models/`); schemas внутри services (не отдельный слой).
|
||||
|
||||
### Fullstack (кратко)
|
||||
|
||||
Backend as above (in `backend/` + `frontend/` separation). Frontend: SvelteKit co-located `*.test.ts` в `src/lib/`, `e2e/*.spec.ts` для Playwright. НЕ смешивать backend код в `frontend/` и наоборот.
|
||||
|
||||
### CLI (кратко)
|
||||
|
||||
`cli.py` (Typer commands) + `core.py` (business logic). Нет api/db/schemas layers. `tests/test_cli.py` + `tests/test_core.py`.
|
||||
|
||||
## 6. Tests
|
||||
|
||||
Что писать (как запускать — в `run-tests` skill). `project-status.py` enforces subset (conftest required, anti-stub, mirror structure); этот раздел объясняет «почему».
|
||||
|
||||
### Типы тестов
|
||||
|
||||
- **Regression** — воспроизводит конкретный баг, который был исправлен. Ссылается на issue/PR (`test_parser_handles_crlf_regression_#227`).
|
||||
- **Integration** — пересекает слои (DB+API, scheduler+DB). Имеет `pytest.mark.integration` + `skipif` opt-in. В `tests/integration/`.
|
||||
- **Unit** — чистая функция/сервис, без DB/сети. В `tests/unit/`.
|
||||
|
||||
### Антипаттерны
|
||||
|
||||
- **Stub files** — `test_*.py` без `def test_*`/`async def test_*` (digital_factory 50/62 файлов). `project-status` WARNs.
|
||||
- **Тесты без assertions** — только `print`/`logger.info`. Каждый тест должен иметь минимум 1 `assert`.
|
||||
- **Тесты ради тестов** — coverage ради coverage, без реальной проверки поведения.
|
||||
|
||||
### Mirror structure (backend)
|
||||
|
||||
- `tests/unit/` ↔ `src/<pkg>/services/` (unit-тесты сервисов)
|
||||
- `tests/api/` ↔ `src/<pkg>/api/v1/routes/` (route-тесты через TestClient)
|
||||
- `tests/integration/` ↔ cross-cutting flows (opt-in)
|
||||
|
||||
### conftest.py
|
||||
|
||||
Обязателен (backend). Shared fixtures: `mock_settings`, `client`, `auth_client`, `create_<entity>` factories.
|
||||
|
|
|
|||
|
|
@ -1852,3 +1852,154 @@ def test_load_pyproject_parse_error_sentinel(tmp_path, ctx):
|
|||
group = ps.check_pyproject(ps.ProjectType.BACKEND, ctx)
|
||||
assert group.overall() == ps.CheckStatus.FAIL
|
||||
assert any("парсинг" in c.detail for c in group.checks)
|
||||
|
||||
|
||||
# ── scattered-models AST check (issue #251) ─────────────────────────────────
|
||||
|
||||
|
||||
def _make_centralized_models_repo(tmp_path: Path) -> None:
|
||||
"""Backend repo with all ORM models centralized in ``src/<pkg>/db/models/``.
|
||||
|
||||
Helper for scattered-models tests: produces the GOOD layout (no WARN).
|
||||
"""
|
||||
_make_backend_repo(tmp_path)
|
||||
pkg = "test_repo"
|
||||
(tmp_path / f"src/{pkg}/db/models/__init__.py").write_text("")
|
||||
(tmp_path / f"src/{pkg}/db/models/user.py").write_text(
|
||||
"from tortoise import Model, fields\nclass User(Model):\n name = fields.CharField()\n"
|
||||
)
|
||||
|
||||
|
||||
def test_scattered_models_centralized_ok(tmp_path, ctx):
|
||||
"""All ORM models in ``src/<pkg>/db/models/`` → no scattered-models WARN."""
|
||||
_make_centralized_models_repo(tmp_path)
|
||||
results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND)
|
||||
assert results == [], f"expected no WARN, got: {results}"
|
||||
|
||||
|
||||
def test_scattered_models_feature_scatter_warn(tmp_path, ctx):
|
||||
"""``class X(Model)`` in ``src/<pkg>/channels/models.py`` → WARN per file."""
|
||||
_make_centralized_models_repo(tmp_path)
|
||||
pkg = "test_repo"
|
||||
scattered_files = [
|
||||
f"src/{pkg}/channels/models.py",
|
||||
f"src/{pkg}/monitor/models.py",
|
||||
f"src/{pkg}/logs/models.py",
|
||||
f"src/{pkg}/models.py",
|
||||
]
|
||||
for rel in scattered_files:
|
||||
path = tmp_path / rel
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
path.write_text(
|
||||
"from tortoise import Model, fields\n"
|
||||
"class Thing(Model):\n name = fields.CharField()\n"
|
||||
)
|
||||
results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND)
|
||||
assert len(results) == 4, f"expected 4 WARN, got {len(results)}: {results}"
|
||||
warned_paths = {r.name for r in results}
|
||||
for rel in scattered_files:
|
||||
assert rel in warned_paths, f"missing WARN for {rel}"
|
||||
assert all(r.status == ps.CheckStatus.WARN for r in results)
|
||||
|
||||
|
||||
def test_scattered_models_in_db_connection_warn(tmp_path, ctx):
|
||||
"""Model in ``src/<pkg>/db/connection.py`` (inside db/, not models/) → WARN."""
|
||||
_make_centralized_models_repo(tmp_path)
|
||||
pkg = "test_repo"
|
||||
(tmp_path / f"src/{pkg}/db/connection.py").write_text(
|
||||
"from tortoise import Model, fields\nclass Internal(Model):\n x = fields.IntField()\n"
|
||||
)
|
||||
results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND)
|
||||
assert len(results) == 1, f"expected 1 WARN, got: {results}"
|
||||
assert results[0].name == f"src/{pkg}/db/connection.py"
|
||||
assert results[0].status == ps.CheckStatus.WARN
|
||||
|
||||
|
||||
def test_scattered_models_in_tests_ok(tmp_path, ctx):
|
||||
"""Model in ``tests/`` (test model) → OK (skip, tests/ not in src/<pkg>/)."""
|
||||
_make_centralized_models_repo(tmp_path)
|
||||
(tmp_path / "tests/test_models.py").write_text(
|
||||
"from tortoise import Model, fields\n"
|
||||
"class TestModel(Model):\n x = fields.IntField()\n"
|
||||
"def test_model(): assert TestModel\n"
|
||||
)
|
||||
results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND)
|
||||
assert results == [], f"expected no WARN, got: {results}"
|
||||
|
||||
|
||||
def test_scattered_models_flat_layout_skip(tmp_path, ctx):
|
||||
"""Flat layout (no ``src/<pkg>/``) → skip check, no results."""
|
||||
_write_pyproject(tmp_path, deps=["fastapi", "uvicorn"])
|
||||
(tmp_path / "src").mkdir()
|
||||
(tmp_path / "src/models.py").write_text(
|
||||
"from tortoise import Model, fields\nclass Flat(Model):\n x = fields.IntField()\n"
|
||||
)
|
||||
results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND)
|
||||
assert results == [], f"flat layout should skip, got: {results}"
|
||||
|
||||
|
||||
def test_scattered_models_cli_skip(tmp_path, ctx):
|
||||
"""CLI (no db) → skip scattered-models check (returns [])."""
|
||||
(tmp_path / "src").mkdir()
|
||||
pkg = tmp_path / "src" / "mycli"
|
||||
pkg.mkdir()
|
||||
(pkg / "__init__.py").write_text("")
|
||||
(pkg / "cli.py").write_text("from tortoise import Model\nclass CliModel(Model):\n pass\n")
|
||||
_write_pyproject(tmp_path, deps=["typer"], scripts={"mycli": "mycli.cli:main"}, name="mycli")
|
||||
results = ps._check_scattered_models(ctx, ps.ProjectType.CLI)
|
||||
assert results == [], f"CLI should skip, got: {results}"
|
||||
|
||||
|
||||
def test_scattered_models_no_tortoise_import_skip(tmp_path, ctx):
|
||||
"""File with ``class X(Model)`` but no tortoise import → skip (false positive guard)."""
|
||||
_make_centralized_models_repo(tmp_path)
|
||||
pkg = "test_repo"
|
||||
(tmp_path / f"src/{pkg}/services/other.py").write_text(
|
||||
"# class from another lib defining its own Model\n"
|
||||
"from some_other_lib import Model\n"
|
||||
"class Other(Model):\n pass\n"
|
||||
)
|
||||
results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND)
|
||||
assert results == [], f"non-tortoise Model should not WARN, got: {results}"
|
||||
|
||||
|
||||
def test_scattered_models_via_import_from_models_module(tmp_path, ctx):
|
||||
"""``class X(Model)`` where ``Model`` comes from own ``db.models`` → WARN."""
|
||||
_make_centralized_models_repo(tmp_path)
|
||||
pkg = "test_repo"
|
||||
(tmp_path / f"src/{pkg}/services/user_service.py").write_text(
|
||||
f"from {pkg}.db.models import Model\nclass ServiceModel(Model):\n pass\n"
|
||||
)
|
||||
results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND)
|
||||
assert len(results) == 1, f"expected 1 WARN, got: {results}"
|
||||
assert results[0].name == f"src/{pkg}/services/user_service.py"
|
||||
assert results[0].status == ps.CheckStatus.WARN
|
||||
|
||||
|
||||
def test_scattered_models_check_structure_integration(tmp_path, ctx):
|
||||
"""``check_structure`` for BACKEND includes scattered-models WARNs."""
|
||||
_make_centralized_models_repo(tmp_path)
|
||||
pkg = "test_repo"
|
||||
(tmp_path / f"src/{pkg}/channels").mkdir(parents=True, exist_ok=True)
|
||||
(tmp_path / f"src/{pkg}/channels/models.py").write_text(
|
||||
"from tortoise import Model, fields\nclass Chan(Model):\n name = fields.CharField()\n"
|
||||
)
|
||||
group = ps.check_structure(ps.ProjectType.BACKEND, ctx)
|
||||
scattered_warns = [
|
||||
c
|
||||
for c in group.checks
|
||||
if "channels/models.py" in c.name and c.status == ps.CheckStatus.WARN
|
||||
]
|
||||
assert scattered_warns, "expected scattered WARN in check_structure, got: " + ", ".join(
|
||||
f"{c.name}={c.status.value}" for c in group.checks
|
||||
)
|
||||
|
||||
|
||||
def test_scattered_models_no_pyproject_skip(tmp_path, ctx):
|
||||
"""No pyproject.toml (cannot resolve package name) → skip, no results."""
|
||||
(tmp_path / "src").mkdir()
|
||||
(tmp_path / "src/whatever.py").write_text(
|
||||
"from tortoise import Model\nclass X(Model):\n pass\n"
|
||||
)
|
||||
results = ps._check_scattered_models(ctx, ps.ProjectType.BACKEND)
|
||||
assert results == [], f"no pyproject should skip, got: {results}"
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue