From b42d2ac3d39b3218041c86ea72a787a78f0e3b49 Mon Sep 17 00:00:00 2001 From: Tyler Matteson Date: Wed, 3 Jun 2026 14:15:22 -0400 Subject: [PATCH] feat: validate doctypes use frappe type checking --- .pre-commit-hooks.yaml | 9 + docs/index.md | 5 + docs/pre_commit_hooks.md | 14 + docs/validate_doctype_python_types.md | 183 +++++++++ pyproject.toml | 1 + .../validate_doctype_python_types.py | 356 ++++++++++++++++++ 6 files changed, 568 insertions(+) create mode 100644 docs/validate_doctype_python_types.md create mode 100644 test_utils/pre_commit/validate_doctype_python_types.py diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml index 996c47e..245bcee 100644 --- a/.pre-commit-hooks.yaml +++ b/.pre-commit-hooks.yaml @@ -130,6 +130,15 @@ pass_filenames: false require_serial: true +- id: validate_doctype_python_types + name: validate_doctype_python_types + description: "Require Frappe auto-generated controller type blocks when export_python_type_annotations is enabled (or use --force)" + entry: validate_doctype_python_types + language: python + always_run: true + pass_filenames: false + require_serial: true + - id: check_code_duplication name: check_code_duplication description: "Check for copy-paste duplication in Python, JavaScript, TypeScript (requires npx)" diff --git a/docs/index.md b/docs/index.md index bcc1663..4fa4dff 100644 --- a/docs/index.md +++ b/docs/index.md @@ -26,6 +26,7 @@ Test Utils is a collection of development tools for [Frappe](https://frappeframe | `bylines` | Add git-history-derived bylines to Markdown files | | `validate_patches` | Validate `patches.txt` ordering and file presence | | `validate_frappe_project` | Validate `pyproject.toml` structure for Frappe apps | +| `validate_doctype_python_types` | Require Frappe auto-generated DocType controller types (or an opt-out comment) when hooks export is enabled ([details](validate_doctype_python_types.md)) | | `check_code_duplication` | Detect copy-paste duplication in Python and JS/TS | | `static_analysis` | Full static analysis suite (see above) | | `code_graph` | Build/query/plot per-app code graph; `--with graph` (+ `--with dev` for `plot`) | @@ -36,6 +37,10 @@ Test Utils is a collection of development tools for [Frappe](https://frappeframe [`validate_customizations.md`](validate_customizations.md) covers the `validate_customizations` hook in depth, including why to avoid fixtures, how to organize customization files, and all validation checks (custom permissions, module attribution, system-generated fields, duplicate detection). +### DocType Python types + +[`validate_doctype_python_types.md`](validate_doctype_python_types.md) covers Frappe’s exported controller types, opt-out comments, pre-commit and CLI usage, and how to test the hook against an existing app repo or bench checkout. + --- ### SQL Registry diff --git a/docs/pre_commit_hooks.md b/docs/pre_commit_hooks.md index 0951b32..c29492c 100644 --- a/docs/pre_commit_hooks.md +++ b/docs/pre_commit_hooks.md @@ -83,3 +83,17 @@ Run mypy on the codebase using a preset configuration hooks: - id: mypy ``` + +### DocType Python types - `validate_doctype_python_types` + +Ensures DocType controllers include Frappe’s auto-generated `TYPE_CHECKING` block or a documented opt-out comment when `export_python_type_annotations` is enabled. Full reference: [`validate_doctype_python_types.md`](validate_doctype_python_types.md). + +``` + - repo: https://github.com/agritheory/test_utils/ + rev: {rev} + hooks: + - id: validate_doctype_python_types + args: ["--app", "."] +``` + +Add `"--interactive"` to `args` locally if you want prompts to run `export_types_to_controller` or insert the skip comment; use `"--force"` to enforce before enabling the hooks flag. diff --git a/docs/validate_doctype_python_types.md b/docs/validate_doctype_python_types.md new file mode 100644 index 0000000..99fb841 --- /dev/null +++ b/docs/validate_doctype_python_types.md @@ -0,0 +1,183 @@ +# DocType Python types — `validate_doctype_python_types` + +Frappe can [auto-generate static type hints](https://frappeframework.com/docs/user/en/basics/doctypes/controllers) on DocType controllers when `export_python_type_annotations = True` is set in `hooks.py`. Saving a DocType in Desk (or calling `export_types_to_controller` on the DocType document) writes a `TYPE_CHECKING` block delimited by: + +- `# begin: auto-generated types` +- `# end: auto-generated types` + +This matches [`frappe.types.exporter`](https://github.com/frappe/frappe/blob/develop/frappe/types/exporter.py). The pre-commit hook checks that every DocType controller either contains that block or an explicit opt-out comment, so teams using exported types do not accidentally commit controllers that fell out of sync with the schema. + +--- + +## When the hook runs + +| Situation | Behavior | +|-----------|----------| +| `export_python_type_annotations` is **not** `True` in `hooks.py` | Exit **0**, message on stderr that the check was skipped. Safe for apps that do not use Frappe’s exporter yet. | +| Flag is **True** (or you pass `--force`) | All matching controllers are scanned; missing block + opt-out → exit **1** with a file list. | + +`hooks.py` is resolved as `//hooks.py` first (e.g. `myapp/myapp/hooks.py`), then `/hooks.py`. + +--- + +## Which files are checked + +Controllers matching: + +`//**/doctype/*/.py` + +where the Python file name equals its parent directory name (Frappe’s usual layout, including nested modules such as `accounts/doctype/sales_invoice/sales_invoice.py`). + +--- + +## Satisfying the check + +**Option A — Frappe-generated block** + +Enable `export_python_type_annotations = True`, then for each listed DocType either re-save it in Desk or run `export_types_to_controller` (e.g. from a bench console). The controller must contain `# begin: auto-generated types`. + +**Option B — Opt-out (documented exception)** + +Add a *whole-line* comment near the top of the controller (after copyright / module docstring is fine). Accepted forms (case-insensitive prefix after strip): + +- `# frappe: skip-python-type-annotations` (preferred in this hook’s interactive fixer) +- `# frappe types: ignore` (convention discussed on Frappe’s exporter PR) + +--- + +## pre-commit configuration + +`--app` must be the **Frappe app repository root**: the directory that contains the inner package folder (same as `static_analysis` / most other test_utils hooks). + +```yaml +- repo: https://github.com/agritheory/test_utils + rev: v1.25.1 # or newer tag containing this hook + hooks: + - id: validate_doctype_python_types + args: ["--app", "."] +``` + +**CI / non-interactive** — use the snippet above. No TTY prompts; failures list paths and exit 1. + +**Local interactive** — optionally add `--interactive` so each violation can be handled with: + +- **[a]** run `frappe.get_doc("DocType", …).export_types_to_controller()` via the bench’s `env/bin/python` (requires a discoverable site), +- **[s]** insert `# frappe: skip-python-type-annotations` after the file header, +- **[n]** leave unchanged (hook still fails), +- **[q]** abort. + +```yaml + - id: validate_doctype_python_types + args: ["--app", ".", "--interactive"] +``` + +**Rollout without enabling hooks first** — enforce scanning even when `export_python_type_annotations` is still off: + +```yaml + - id: validate_doctype_python_types + args: ["--app", ".", "--force"] +``` + +--- + +## CLI flags + +| Flag | Meaning | +|------|---------| +| `--app PATH` | App repo root. Default: `.` | +| `--force` | Run checks even if `export_python_type_annotations` is not enabled. | +| `--interactive` | Prompt per violation (only when stdin is a TTY). | +| `--site NAME` | Bench site for interactive export; overrides `FRAPPE_SITE` / config defaults. | + +### Site resolution for interactive export + +Interactive **[a]** needs a bench layout (`sites/`, `apps/`, `env/`) reachable by walking up from `--app`, and a site name from, in order: + +1. `--site` +2. Environment variable `FRAPPE_SITE` +3. `default_site` in `sites/common_site_config.json` +4. `sites/currentsite.txt` + +--- + +## Running without pre-commit + +From a machine that has `test_utils` installed (or a Poetry shell inside this repo): + +```bash +validate_doctype_python_types --app /path/to/bench/apps/myapp +``` + +Or: + +```bash +cd /path/to/test_utils && poetry run python -m test_utils.pre_commit.validate_doctype_python_types \ + --app /path/to/bench/apps/myapp +``` + +Exit codes: `0` ok / skipped, `1` violations (or abort in interactive mode). + +--- + +## Testing on an existing Frappe app (local `test_utils` clone) + +Use this while the hook is only on a branch or not yet released. + +### 1. One-off run from your app repo + +From the **app root** (directory containing `myapp/myapp/`): + +```bash +cd /path/to/bench/apps/myapp +/path/to/test_utils/.venv/bin/python -m test_utils.pre_commit.validate_doctype_python_types --app . +``` + +Or after `pip install /path/to/test_utils` (or `poetry add --editable`), run `validate_doctype_python_types --app .`. + +Add `--force` if you have not enabled `export_python_type_annotations` yet. Add `--interactive` in a real terminal to try export or auto-insert skip comments. + +### 2. Through pre-commit using your clone + +Point `repo` at your local git checkout and pin `rev` to a commit or branch tip that exists **in that repo**: + +```yaml +- repo: /home/you/projects/test_utils + rev: your-branch-name + hooks: + - id: validate_doctype_python_types + args: ["--app", "."] +``` + +Then: + +```bash +cd /path/to/bench/apps/myapp +pre-commit autoupdate --repo /home/you/projects/test_utils # optional, refreshes cache +pre-commit run validate_doctype_python_types --all-files +``` + +### 3. `pre-commit try-repo` (quick smoke test) + +From the **target app** root: + +```bash +cd /path/to/bench/apps/myapp +pre-commit try-repo /path/to/test_utils validate_doctype_python_types --all-files -v +``` + +Note: `try-repo` uses the hook defaults (`--app` defaults to `.`), which is correct when you run the command from the app root. + +--- + +## Troubleshooting + +- **Hook always skips** — Ensure `export_python_type_annotations = True` is a literal assignment in `hooks.py`, or pass `--force`. +- **Interactive export does nothing** — Confirm `export_python_type_annotations` is enabled for that app; `export_types_to_controller` returns early otherwise. Confirm site and bench paths. +- **False “missing types” after export** — Run formatter (Black, etc.); the block must still contain the exact marker line `# begin: auto-generated types`. + +--- + +## See also + +- [Controllers — Frappe docs](https://frappeframework.com/docs/user/en/basics/doctypes/controllers) +- [`pre_commit_hooks.md`](pre_commit_hooks.md) for the full hook list diff --git a/pyproject.toml b/pyproject.toml index 3ae3138..356e33f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -24,6 +24,7 @@ sql_pipeline = "test_utils.utils.sql_pipeline:main" bylines = "test_utils.pre_commit.bylines:main" validate_patches = "test_utils.pre_commit.validate_patches:main" validate_frappe_project = "test_utils.pre_commit.validate_frappe_project:main" +validate_doctype_python_types = "test_utils.pre_commit.validate_doctype_python_types:main" check_code_duplication = "test_utils.pre_commit.check_code_duplication:main" static_analysis = "test_utils.pre_commit.static_analysis:main" semgrep_fmt = "test_utils.pre_commit.semgrep_fmt:main" diff --git a/test_utils/pre_commit/validate_doctype_python_types.py b/test_utils/pre_commit/validate_doctype_python_types.py new file mode 100644 index 0000000..5673501 --- /dev/null +++ b/test_utils/pre_commit/validate_doctype_python_types.py @@ -0,0 +1,356 @@ +""" +Ensure DocType controllers include Frappe auto-generated TYPE_CHECKING blocks when +`export_python_type_annotations` is enabled, or an explicit opt-out comment. + +Detection matches Frappe's exporter markers (see frappe.types.exporter). +""" + +from __future__ import annotations + +import argparse +import ast +import json +import os +import pathlib +import subprocess +import sys +from collections.abc import Sequence + +AUTO_TYPES_BEGIN = "# begin: auto-generated types" + +# Recognised opt-outs (Frappe PR #21776 suggested a file-level ignore; we accept both.) +SKIP_COMMENT_CANONICAL = "# frappe: skip-python-type-annotations" +SKIP_COMMENT_ALT = "# frappe types: ignore" + + +def find_bench_root(start: pathlib.Path) -> pathlib.Path | None: + for p in [start] + list(start.parents): + if (p / "sites").is_dir() and (p / "apps").is_dir() and (p / "env").is_dir(): + return p + return None + + +def resolve_hooks_path(app_path: pathlib.Path) -> pathlib.Path | None: + h = app_path / app_path.name / "hooks.py" + if h.exists(): + return h + h2 = app_path / "hooks.py" + if h2.exists(): + return h2 + return None + + +def hooks_enables_export_python_types(hooks_file: pathlib.Path) -> bool: + try: + tree = ast.parse(hooks_file.read_text(encoding="utf-8")) + except (OSError, SyntaxError): + return False + for node in ast.walk(tree): + if not isinstance(node, ast.Assign): + continue + for target in node.targets: + if isinstance(target, ast.Name) and target.id == "export_python_type_annotations": + val = node.value + if isinstance(val, ast.Constant): + return bool(val.value) + if isinstance(val, ast.Name) and val.id == "True": + return True + return False + + +def line_is_skip_comment(line: str) -> bool: + s = line.strip() + low = s.lower() + if low.startswith(SKIP_COMMENT_CANONICAL.lower()): + return True + if low.startswith(SKIP_COMMENT_ALT.lower()): + return True + return False + + +def source_has_auto_types_or_skip(text: str) -> bool: + if AUTO_TYPES_BEGIN in text: + return True + return any(line_is_skip_comment(ln) for ln in text.splitlines()) + + +def insert_skip_comment_after_header(content: str, comment: str) -> str: + lines = content.splitlines(keepends=True) + idx = 0 + if lines and lines[0].startswith("#!"): + idx = 1 + while idx < len(lines): + line = lines[idx] + stripped = line.strip() + if stripped.startswith("#") and not line_is_skip_comment(line): + idx += 1 + continue + if stripped.startswith('"""'): + delim = '"""' + if stripped.count(delim) >= 2: + idx += 1 + continue + idx += 1 + while idx < len(lines) and delim not in lines[idx]: + idx += 1 + idx += 1 + continue + if stripped.startswith("'''"): + delim = "'''" + if stripped.count(delim) >= 2: + idx += 1 + continue + idx += 1 + while idx < len(lines) and delim not in lines[idx]: + idx += 1 + idx += 1 + continue + break + block = f"{comment}\n\n" + if idx == 0: + return block + content + return "".join(lines[:idx]) + block + "".join(lines[idx:]) + + +def apply_skip_comment(controller_path: pathlib.Path) -> None: + text = controller_path.read_text(encoding="utf-8") + if source_has_auto_types_or_skip(text): + return + updated = insert_skip_comment_after_header(text, SKIP_COMMENT_CANONICAL) + controller_path.write_text(updated, encoding="utf-8") + print(f"Wrote opt-out comment in {controller_path}", file=sys.stderr) + + +def read_doctype_name(doctype_dir: pathlib.Path) -> str | None: + json_path = doctype_dir / f"{doctype_dir.name}.json" + if not json_path.exists(): + return None + try: + data = json.loads(json_path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + return None + name = data.get("name") + return name if isinstance(name, str) else None + + +def iter_doctype_controllers(app_path: pathlib.Path): + app_pkg = app_path / app_path.name + if not app_pkg.is_dir(): + return + for py in sorted(app_pkg.rglob("doctype/*/*.py")): + if py.name == "__init__.py": + continue + parent = py.parent + if py.name != f"{parent.name}.py": + continue + yield parent.name, py + + +def collect_violations(app_path: pathlib.Path) -> list[tuple[str, pathlib.Path]]: + out: list[tuple[str, pathlib.Path]] = [] + for scrubbed, py_path in iter_doctype_controllers(app_path): + try: + text = py_path.read_text(encoding="utf-8") + except OSError: + continue + if source_has_auto_types_or_skip(text): + continue + out.append((scrubbed, py_path)) + return out + + +def resolve_site(bench_root: pathlib.Path, site_arg: str | None) -> str | None: + if site_arg: + return site_arg + env_site = os.environ.get("FRAPPE_SITE") + if env_site: + return env_site + cfg = bench_root / "sites" / "common_site_config.json" + if cfg.exists(): + try: + site = json.loads(cfg.read_text(encoding="utf-8")).get("default_site") + if isinstance(site, str) and site: + return site + except (OSError, json.JSONDecodeError): + return None + cur = bench_root / "sites" / "currentsite.txt" + if cur.is_file(): + raw = cur.read_text(encoding="utf-8").strip() + if raw: + return raw + return None + + +def bench_export_types( + bench_root: pathlib.Path, site: str, doctype_record_name: str +) -> tuple[bool, str]: + py_bin = bench_root / "env" / "bin" / "python" + if not py_bin.is_file(): + return False, f"no interpreter at {py_bin}" + sites_path = str(bench_root / "sites") + code = ( + "import frappe\n" + f"frappe.init(site={site!r}, sites_path={sites_path!r})\n" + "frappe.connect()\n" + f'frappe.get_doc("DocType", {doctype_record_name!r}).export_types_to_controller()\n' + ) + proc = subprocess.run( + [str(py_bin), "-c", code], + cwd=str(bench_root), + capture_output=True, + text=True, + timeout=120, + ) + if proc.returncode != 0: + err = (proc.stderr or proc.stdout or "").strip() + return False, err or f"exit {proc.returncode}" + return True, "" + + +def prompt_for_violation(doctype_scrubbed: str, py_path: pathlib.Path) -> str: + print( + f"\nDocType controller missing Frappe auto-generated types: {doctype_scrubbed}", + file=sys.stderr, + ) + print(f" File: {py_path}", file=sys.stderr) + print( + " [a] run export_types_to_controller via bench env (needs site; " + "set FRAPPE_SITE or ensure sites/common_site_config.json has default_site)", + file=sys.stderr, + ) + print(f" [s] insert opt-out line: {SKIP_COMMENT_CANONICAL}", file=sys.stderr) + print(" [n] leave as-is (hook will fail)", file=sys.stderr) + print(" [q] abort", file=sys.stderr) + while True: + try: + c = input("Choice [a/s/n/q]: ").strip().lower() + except EOFError: + return "n" + if c in ("a", "s", "n", "q"): + return c + + +def main(argv: Sequence[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description=( + "Require Frappe auto-generated controller types (or an opt-out comment) " + "when export_python_type_annotations is enabled in hooks.py." + ) + ) + parser.add_argument( + "--app", + default=".", + help="Frappe app root (repository root containing the inner package directory). Default: cwd.", + ) + parser.add_argument( + "--force", + action="store_true", + help="Check controllers even if export_python_type_annotations is not set in hooks.py.", + ) + parser.add_argument( + "--interactive", + action="store_true", + help="Prompt per violation (export via bench, add skip comment, or fail). Requires a TTY.", + ) + parser.add_argument( + "--site", + default=None, + help="Bench site name for --interactive export (overrides FRAPPE_SITE / default_site).", + ) + args = parser.parse_args(list(argv) if argv is not None else None) + + app_path = pathlib.Path(args.app).resolve() + if not app_path.is_dir(): + print(f"Error: --app path does not exist: {app_path}", file=sys.stderr) + return 1 + + hooks_path = resolve_hooks_path(app_path) + hook_on = hooks_path is not None and hooks_enables_export_python_types(hooks_path) + + if not args.force and not hook_on: + if hooks_path is None: + print( + "validate_doctype_python_types: no hooks.py found; use --force to check anyway.", + file=sys.stderr, + ) + else: + print( + "validate_doctype_python_types: export_python_type_annotations is not enabled in " + f"{hooks_path}; skipping. Enable it or pass --force.", + file=sys.stderr, + ) + return 0 + + violations = collect_violations(app_path) + if not violations: + return 0 + + bench_root = find_bench_root(app_path) + site = resolve_site(bench_root, args.site) if bench_root else None + + if args.interactive and sys.stdin.isatty(): + still_failed: list[tuple[str, pathlib.Path]] = [] + for scrubbed, py_path in violations: + choice = prompt_for_violation(scrubbed, py_path) + if choice == "q": + print("Aborted.", file=sys.stderr) + return 1 + if choice == "s": + apply_skip_comment(py_path) + continue + if choice == "a": + if not bench_root: + print("No bench root found (need sites/, apps/, env/).", file=sys.stderr) + still_failed.append((scrubbed, py_path)) + continue + if not site: + print( + "No site resolved; set FRAPPE_SITE or default_site in sites/common_site_config.json.", + file=sys.stderr, + ) + still_failed.append((scrubbed, py_path)) + continue + dt_name = read_doctype_name(py_path.parent) or scrubbed.replace("_", " ").title() + ok, err = bench_export_types(bench_root, site, dt_name) + if not ok: + print(f"export_types_to_controller failed: {err}", file=sys.stderr) + still_failed.append((scrubbed, py_path)) + continue + text = py_path.read_text(encoding="utf-8") + if AUTO_TYPES_BEGIN not in text: + print( + "export ran but controller still has no auto-generated block; " + "ensure export_python_type_annotations is True for this app.", + file=sys.stderr, + ) + still_failed.append((scrubbed, py_path)) + continue + still_failed.append((scrubbed, py_path)) + violations = still_failed + + if not violations: + return 0 + + print( + "DocType controllers without Frappe auto-generated types " + f"(expected `{AUTO_TYPES_BEGIN}` or `{SKIP_COMMENT_CANONICAL}` / `{SKIP_COMMENT_ALT}`):", + file=sys.stderr, + ) + for scrubbed, py_path in violations: + print(f" {scrubbed}: {py_path}", file=sys.stderr) + print( + "\nFix: enable export_python_type_annotations in hooks.py, re-save the DocType in Desk " + "(or run export_types_to_controller), or add an opt-out comment at the top of the controller.", + file=sys.stderr, + ) + if bench_root and site and (bench_root / "env" / "bin" / "python").is_file(): + print( + f"\nBench detected at {bench_root} (site {site!r}). " + "You can run this hook with --interactive to export or add the skip comment.", + file=sys.stderr, + ) + return 1 + + +if __name__ == "__main__": + sys.exit(main())