From 266ae7f5ffee27b0689e6fd5995690f2128d7b32 Mon Sep 17 00:00:00 2001 From: Tyler Matteson Date: Mon, 23 Feb 2026 17:15:10 -0500 Subject: [PATCH] feat: sql rewriter fixes --- docs/pre_commit_migration_plan.md | 123 ------------------ .../pre_commit/sql_rewriter_functions.py | 31 ++++- test_utils/utils/sql_registry/converter.py | 2 + test_utils/utils/sql_registry/registry.py | 40 +++--- test_utils/utils/sql_registry/scanner.py | 9 ++ 5 files changed, 58 insertions(+), 147 deletions(-) delete mode 100644 docs/pre_commit_migration_plan.md diff --git a/docs/pre_commit_migration_plan.md b/docs/pre_commit_migration_plan.md deleted file mode 100644 index af5d782..0000000 --- a/docs/pre_commit_migration_plan.md +++ /dev/null @@ -1,123 +0,0 @@ -# Pre-commit Migration Plan (version-15 apps) - -Standardize pre-commit hooks and pyproject.toml across Frappe apps. - -## Standard Configuration - -### Pre-commit hooks (order) -1. **pre-commit-hooks** (v5.0.0): trailing-whitespace, check-yaml, no-commit-to-branch (version-15, version-16), check-merge-conflict, check-ast, check-json, check-toml, debug-statements -2. **codespell** (v2.4.1): args `--ignore-words-list notin`, exclude `yarn.lock|poetry.lock` -3. **pyupgrade** (v3.19.1): args `--py310-plus` -4. **black** (agritheory/black): standard -5. **flake8** (7.2.0): flake8-bugbear -6. **test_utils** (v1.15.4): - - update_pre_commit_config - - validate_frappe_project - - validate_copyright (files `\.(js|ts|py|md)$`, args `--app {app_name}`) - - bylines (exclude README.md, CHANGELOG.md) - - clean_customized_doctypes (args `--app {app_name}`) - - validate_customizations - - patch_linters (args `--app {app_name}`) - - track_overrides (args `--directory .`, `--app {app_name}`, `--base-branch version-15`; set `GH_TOKEN` or `GITHUB_TOKEN` for GitHub API auth) - - check_code_duplication (args `--max-clones 60`, `--max-percentage 5.0`) -7. **prettier** (mirrors-prettier v3.1.0 or local npx) - after test_utils - -### pyproject.toml requirements -- `[project]` with name, version, authors, description, readme, requires-python -- `[tool.bench.frappe-dependencies]` with bounded frappe (and erpnext if used) -- No hybrid config: Flit OR Poetry, not both -- Version consistency: `[project.version]` as source of truth; remove duplicate `[tool.poetry]` metadata -- `[project.dynamic] = ["dependencies"]` when using `[tool.poetry.dependencies]` exclusively - -### pytest CI (apps with tests) -- Use bench’s `[tool.bench.dev-dependencies]` for test tools (pytest, pytest-cov, pytest-order, etc.) -- Use pip-style version specifiers (`~=x.y.z`, `>=x.y.z`), not Poetry `^` — bench passes these directly to pip -- CI helper scripts must run `bench setup requirements --dev` after `bench setup requirements --python` -- Bench reads only `[tool.bench.dev-dependencies]`; Poetry’s `[tool.poetry.group.dev.dependencies]` is ignored by bench - -### Pytest CI audit (2025-02) - -| App | Had `[tool.bench.dev-dependencies]` | Ran `--dev` in CI | Fixed | -|-----|------------------------------------|-------------------|-------| -| check_run | ✗ | ✗ | ✓ | -| electronic_payments | ✗ | ✗ | ✓ | -| taxjar_erpnext | ✗ | ✗ | ✓ | -| inventory_tools | ✗ | ✗ | ✓ | -| beam | ✗ | ✓ | ✓ (added bench dev-deps) | -| fleet | ✗ | ✓ | ✓ (added bench dev-deps) | -| cloud_storage | ✗ | ✓ | ✓ (added bench dev-deps) | -| approvals | ✗ | ✓ | ✓ (added bench dev-deps) | -| saml | ✗ | ✓ + `bench pip install` hack | ✓ (replaced with bench dev-deps) | -| shipstation_integration | ✓ | ✓ | — | -| fab | — | ✓ | — (uses `run-tests`, not pytest) | - -## Progress Summary - -- **Complete:** 12 apps — fleet, approvals, saml, fab, greensight, cloud_storage, inventory_tools, check_run, electronic_payments, beam, taxjar_erpnext (+ time_and_expense poetry only) -- **WIP:** shipstation_integration (poetry lock blocked by test_utils Python \<3.14) -- **Remaining:** ~8 apps - -## App Status - -| App | Bench | Pre-commit | pyproject.toml | poetry check | Notes | -|-----|-------|------------|----------------|--------------|-------| -| fleet | hawthorn | ✓ | ✓ | ✓ | Complete; added track_overrides, patches/ dir | -| shipstation_integration | quercus | ✓ | ✓ | ✗ | WIP: test_utils requires Python <3.14 | -| approvals | cranberry | ✓ | ✓ | ✓ | Complete; removed duplicate [tool.poetry] metadata | -| saml | cranberry | ✓ | ✓ | ✓ | Done | -| fab | cranberry | ✓ | ✓ | ✓ | Complete; removed duplicate [tool.poetry] metadata | -| greensight | cranberry | ✓ | ✓ | ✓ | Complete; added project.version | -| time_and_expense | cranberry | ? | ✓ | ✓ | poetry lock OK; pre-commit needs review | -| cloud_storage | uhdei | ✓ | ✓ | ✓ | Complete | -| inventory_tools | salix | ✓ | ✓ | ✓ | Complete | -| frappe_vault | uhdei | | | | Pending | -| electronic_payments | poplar | ✓ | ✓ | ✓ | Complete | -| check_run | cedar | ✓ | ✓ | ✓ | Complete | -| beam | pinyon | ✓ | ✓ | ✓ | Complete | -| upro_erp | | | | | Pending | -| upro_erp_integ | | | | | Pending | -| washmoreerp | | | | | Pending | -| communications | | | | | Pending | -| taxjar_erpnext | magnolia | ✓ | ✓ | ✓ | Complete; created from scratch | - -## GitHub Actions (GHA) Inventory - -| App | lint | pytest | release | generate-changelog | backport | code-dup | overrides | Notes | -|-----|------|--------|---------|-------------------|----------|----------|-----------|-------| -| fleet | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ (agritheory) | Full set | -| shipstation_integration | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | Refactored to AgriTheory standard | -| approvals | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | — | | -| saml | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ (agritheory) | Added backport | -| fab | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | — | Added backport | -| greensight | ✓ | — | ✓ | ✓ | — | ✓ | — | No tests (customer app, mostly customizations); added changelog | -| cloud_storage | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ (agritheory) | Complete; added changelog | -| inventory_tools | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ (agritheory) | Added track_overrides | -| check_run | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ (agritheory) | Added overrides | -| electronic_payments | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ (agritheory) | Complete; added backport | -| beam | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ (agritheory) | Complete; added overrides, changelog | -| taxjar_erpnext | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ | ✓ (agritheory) | Created from scratch; added backport | -| time_and_expense | — | — | — | — | — | — | — | Customer GitLab; no GHAs required | - -### Code duplication audit (2025-02) - -Added `agritheory/test_utils/actions/code_duplication` to: check_run, electronic_payments, beam, taxjar_erpnext, fleet, cloud_storage, approvals, saml, fab, shipstation_integration, greensight (inventory_tools and test_utils had it already). - -### Backport audit (2025-02) - -| App | Had backport | Fixed | -|-----|--------------|-------| -| electronic_payments | ✗ | ✓ | -| taxjar_erpnext | ✗ | ✓ | -| saml | ✗ | ✓ | -| fab | ✗ | ✓ | -| test_utils | ✗ | ✓ | -| check_run, beam, fleet, cloud_storage, approvals, inventory_tools, shipstation_integration | ✓ | — | - -### Standard set -- **lint**: pre-commit/action or equivalent (black, mypy, prettier) -- **pytest**: Frappe bench + MariaDB (skip if app has no tests, e.g. greensight) -- **release**: python-semantic-release on version-15 (and version-14 if applicable) -- **generate-changelog**: agritheory/test_utils/actions/generate_changelog -- **backport**: tibdex/backport workflow -- **overrides**: agritheory/test_utils/actions/track_overrides (with `app:` input) -- **code-duplication**: agritheory/test_utils/actions/code_duplication (see docs/actions/code-duplication.md) diff --git a/test_utils/pre_commit/sql_rewriter_functions.py b/test_utils/pre_commit/sql_rewriter_functions.py index cf2e0da..55c46bd 100644 --- a/test_utils/pre_commit/sql_rewriter_functions.py +++ b/test_utils/pre_commit/sql_rewriter_functions.py @@ -147,8 +147,11 @@ def rewrite_sql(self, call_id: str, dry_run: bool = True, backup: bool = True): call = matching_calls[0] - # Check if this call is flagged for manual review - if call.query_builder_equivalent and "# MANUAL:" in call.query_builder_equivalent: + # Check if this call is flagged for manual review or has a conversion error + if call.query_builder_equivalent and ( + "# MANUAL:" in call.query_builder_equivalent + or "# Error" in call.query_builder_equivalent + ): print( f"\n{Colors.YELLOW}Skipping {call.call_id[:12]} - flagged for manual review:{Colors.RESET}" ) @@ -199,9 +202,11 @@ def rewrite_sql(self, call_id: str, dry_run: bool = True, backup: bool = True): # Write the modified content file_path.write_text(formatted_content, encoding="utf-8") - # Update registry + # Update registry and re-scan the modified file so line numbers stay current + # for any subsequent rewrites on the same file in this session call.implementation_type = "query_builder" call.notes = f"Converted by sql_rewriter on {call.updated_at}" + self.registry.scan_file(file_path) self.registry.save_registry() print(f"{Colors.GREEN}Successfully converted SQL to Query Builder{Colors.RESET}") @@ -235,8 +240,11 @@ def rewrite_batch( ] if matching: call = matching[0] - # Skip calls flagged for manual review - if call.query_builder_equivalent and "# MANUAL:" in call.query_builder_equivalent: + # Skip calls flagged for manual review or with conversion errors + if call.query_builder_equivalent and ( + "# MANUAL:" in call.query_builder_equivalent + or "# Error" in call.query_builder_equivalent + ): skipped_manual.append(call) continue calls_to_rewrite.append(call) @@ -395,8 +403,17 @@ def replace_sql_in_content(self, content: str, call) -> str: # Empty lines indented_replacement.append("") - # Replace the lines - new_lines = lines[:start_line] + indented_replacement + lines[end_line + 1 :] + # If this call was the iterable in a for loop, reconstruct the for statement + if call.variable_name and call.variable_name.startswith("__for_"): + loop_var = call.variable_name[ + len("__for_") : -2 + ] # strip __for_ prefix and __ suffix + for_line = indent_str + f"for {loop_var} in result:" + new_lines = ( + lines[:start_line] + indented_replacement + [for_line] + lines[end_line + 1 :] + ) + else: + new_lines = lines[:start_line] + indented_replacement + lines[end_line + 1 :] return "\n".join(new_lines) diff --git a/test_utils/utils/sql_registry/converter.py b/test_utils/utils/sql_registry/converter.py index ef4f05e..8b39afb 100644 --- a/test_utils/utils/sql_registry/converter.py +++ b/test_utils/utils/sql_registry/converter.py @@ -440,6 +440,8 @@ def convert_select_to_qb( result_prefix = "yield " elif variable_name == "__expr__": result_prefix = "" + elif variable_name and variable_name.startswith("__for_"): + result_prefix = "result = " elif variable_name: result_prefix = f"{variable_name} = " else: diff --git a/test_utils/utils/sql_registry/registry.py b/test_utils/utils/sql_registry/registry.py index 5bb14b6..74ba9d3 100644 --- a/test_utils/utils/sql_registry/registry.py +++ b/test_utils/utils/sql_registry/registry.py @@ -381,7 +381,10 @@ def generate_report(self) -> str: for call in calls.values(): by_type[call.implementation_type] = by_type.get(call.implementation_type, 0) + 1 if call.query_builder_equivalent: - if "# MANUAL:" in call.query_builder_equivalent: + if ( + "# MANUAL:" in call.query_builder_equivalent + or "# Error" in call.query_builder_equivalent + ): manual_count += 1 elif "frappe.get_all(" in call.query_builder_equivalent: orm_count += 1 @@ -451,22 +454,25 @@ def generate_report(self) -> str: report += "| Call ID | Status | Line | Function | SQL Preview |\n" report += "|---------|--------|------|----------|-------------|\n" - for call in sorted_calls: - status = "✅" - if call.query_builder_equivalent: - if "# MANUAL:" in call.query_builder_equivalent: - status = "🔧" - elif "frappe.get_all(" in call.query_builder_equivalent: - status = "💡" - elif "# TODO" in call.query_builder_equivalent: - status = "⚠️" - - sql_preview = call.sql_query.replace("\n", " ").strip()[:50] - if len(call.sql_query) > 50: - sql_preview += "..." - sql_preview = sql_preview.replace("|", "\\|") - func_name = call.function_context[:25] if call.function_context else "" - report += f"| `{call.call_id[:8]}` | {status} | {call.line_number} | {func_name} | {sql_preview} |\n" + for call in sorted_calls: + status = "✅" + if call.query_builder_equivalent: + if ( + "# MANUAL:" in call.query_builder_equivalent + or "# Error" in call.query_builder_equivalent + ): + status = "🔧" + elif "frappe.get_all(" in call.query_builder_equivalent: + status = "💡" + elif "# TODO" in call.query_builder_equivalent: + status = "⚠️" + + sql_preview = call.sql_query.replace("\n", " ").strip()[:50] + if len(call.sql_query) > 50: + sql_preview += "..." + sql_preview = sql_preview.replace("|", "\\|") + func_name = call.function_context[:25] if call.function_context else "" + report += f"| `{call.call_id[:8]}` | {status} | {call.line_number} | {func_name} | {sql_preview} |\n" report += "\n" diff --git a/test_utils/utils/sql_registry/scanner.py b/test_utils/utils/sql_registry/scanner.py index 03a230e..68d443a 100644 --- a/test_utils/utils/sql_registry/scanner.py +++ b/test_utils/utils/sql_registry/scanner.py @@ -185,6 +185,13 @@ def extract_variable_name(tree: ast.AST, call_node: ast.Call) -> str | None: if node.value and node_contains(node.value, call_node): return "__yield__" + for node in ast.walk(tree): + if isinstance(node, ast.For): + if node_contains(node.iter, call_node): + if isinstance(node.target, ast.Name): + return f"__for_{node.target.id}__" + return "__for_iter__" + for node in ast.walk(tree): if isinstance(node, ast.Expr): if node_contains(node.value, call_node): @@ -452,6 +459,8 @@ def convert_select_to_orm( result_prefix = "yield " elif variable_name == "__expr__": result_prefix = "" + elif variable_name and variable_name.startswith("__for_"): + result_prefix = "result = " elif variable_name: result_prefix = f"{variable_name} = " else: