diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000..4f9d1fc --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,29 @@ +language: en-US + +reviews: + profile: chill + request_changes_workflow: false + high_level_summary: true + review_status: true + auto_review: + enabled: true + drafts: false + path_filters: + - "!dist/**" + - "!website/build/**" + - "!website/.docusaurus/**" + path_instructions: + - path: "py_sql_cleaner/**/*.py" + instructions: > + Review against the layered architecture in docs/architecture/README.md. + Keep SQLGlot isolated in adapters, keep Typer and Rich in cli, and keep + application code dependent on formatter ports instead of concrete + adapters. + - path: "tests/**/*.py" + instructions: > + Prefer focused behavior tests that cover CLI behavior, formatter + behavior, and safety around skipped f-strings or Jinja-like templates. + - path: "docs/**" + instructions: > + Keep development documentation concise and consistent with + CONTRIBUTING.md and docs/architecture/README.md. diff --git a/AGENTS.md b/AGENTS.md index c3262da..f19be1a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,6 +1,14 @@ # Coding Agent Instructions -Follow the package architecture in `docs/architecture/README.md`. +Start here, then open the referenced docs only when they are relevant to the +change. + +## Project Map + +- Architecture: `docs/architecture/README.md` +- Branch workflow: `docs/development/branching.md` +- Test layout: `tests/README.md` +- Contributor workflow: `CONTRIBUTING.md` ## Dependency Rules diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 05e5700..cd2f7dd 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -46,6 +46,8 @@ Check built distributions: ## Contribution Guidelines - Keep changes focused. +- Use the branching strategy in `docs/development/branching.md`; do not push + directly to `main`. - Add or update tests for behavior changes. - Keep package dependencies flowing in the direction documented in `docs/architecture/README.md`. @@ -65,15 +67,21 @@ git config core.hooksPath .githooks The pre-push hook runs `scripts/check`, matching the local harness and CI. +## Code Review Tooling + +CodeRabbit repository settings live in `.coderabbit.yaml`. Install the CodeRabbit +GitHub App on this public repository to enable automated pull request reviews. + ## Scope The current focus is: - Python files - triple-quoted SQL strings -- SQLGlot-backed SQL formatting, currently defaulting to the Redshift dialect +- SQLGlot-backed SQL formatting, defaulting to generic SQL with `--dialect` + support for explicitly enabled database-specific formatting - formatting - extracting SQL into `.sql` files -Please open an issue before starting large features such as new dialect support, -template handling, or formatter backend changes. +Please open an issue before starting large features such as template handling, +formatter backend changes, or new dialect support. diff --git a/README.md b/README.md index 4aff7bc..ce09073 100644 --- a/README.md +++ b/README.md @@ -4,8 +4,9 @@ embedded in Python files. It is built for Python codebases where long SQL queries are written directly -inside triple-quoted Python strings. The current MVP defaults to the Redshift -dialect through SQLGlot, with room for broader dialect support over time. +inside triple-quoted Python strings. The current MVP uses SQLGlot for formatting, +defaults to SQLGlot's generic dialect, and can format with database-specific +dialects that this project has explicitly enabled via `--dialect`. ```python query = """ @@ -20,6 +21,10 @@ file. `py-sql-cleaner` is an early MVP. It uses SQLGlot internally for best-effort SQL formatting. It does not connect to databases and does not execute SQL. +Dialect support means SQLGlot parser/formatter mode selection, not exhaustive +database validation. Redshift command-style statements such as `COPY` and +`UNLOAD` are preserved rather than reformatted to avoid changing load/export +options. > [!NOTE] > `py-sql-cleaner` is conservative by default: f-strings and Jinja-like templates are @@ -33,6 +38,7 @@ formatting. It does not connect to databases and does not execute SQL. - Detect common SQL variable names such as `sql`, `query`, `*_sql`, and `*_query` - Skip unsafe blocks, including f-strings and Jinja-like templates, by default +- Support explicit dialect selection with `--dialect` / `-d` - Support `check` mode for CI - Support `dry-run` mode before rewriting files @@ -77,13 +83,22 @@ uvx py-sql-cleaner --help py-sql-cleaner format jobs/load_users.py ``` -4. Extract embedded SQL into `.sql` files: +4. Format with a database-specific dialect: + + ```bash + py-sql-cleaner format jobs/load_users.py -d redshift + ``` + + Currently enabled dialects are `generic`, `mysql`, `postgres`, and + `redshift`. + +5. Extract embedded SQL into `.sql` files: ```bash py-sql-cleaner extract jobs/load_users.py --out-dir sql ``` -5. Check formatting for CI: +6. Check formatting for CI: ```bash py-sql-cleaner check jobs/load_users.py @@ -195,6 +210,7 @@ WHERE ds = '{{ ds }}' | `format` | Format embedded SQL in place | `py-sql-cleaner format jobs/load_users.py` | | `check` | Check whether embedded SQL is formatted | `py-sql-cleaner check jobs/load_users.py` | | `extract` | Extract embedded SQL into `.sql` files | `py-sql-cleaner extract jobs/load_users.py --out-dir sql` | +| `dialects` | List accepted dialect values | `py-sql-cleaner dialects` | ## Documentation @@ -211,7 +227,8 @@ The current focus is: - Python files - triple-quoted SQL strings -- SQLGlot-backed SQL formatting, currently defaulting to the Redshift dialect +- SQLGlot-backed SQL formatting, defaulting to generic SQL with `--dialect` + support for explicitly enabled database-specific formatting - formatting - extracting SQL into `.sql` files diff --git a/docs/development/branching.md b/docs/development/branching.md new file mode 100644 index 0000000..33e7d99 --- /dev/null +++ b/docs/development/branching.md @@ -0,0 +1,97 @@ +# Branching Strategy + +This repository uses a lightweight GitHub Flow. Keep the model simple: +`main` is protected, short-lived branches carry changes, and every change lands +through a pull request. + +```mermaid +gitGraph + commit id: "main"s + branch feature/add-postgres-dialect + checkout feature/add-postgres-dialect + commit id: "focused change" + commit id: "tests/docs" + checkout main + merge feature/add-postgres-dialect id: "PR merge" + branch fix/preserve-copy-options + checkout fix/preserve-copy-options + commit id: "bug fix" + checkout main + merge fix/preserve-copy-options id: "PR merge" +``` + +## Branches + +- `main` is the only long-lived branch. +- `main` should always be releasable. +- Do not push commits directly to `main`. +- Do not create a permanent `develop` branch. This project is small enough that + a second integration branch would add delay without improving safety. +- Create short-lived branches from the latest `main`. + +Use these branch name prefixes: + +- `feature/` for user-facing behavior or new capabilities. +- `fix/` for bug fixes. +- `docs/` for documentation-only changes. +- `chore/` for maintenance, tooling, or dependency updates. +- `release/` only when release preparation needs multiple commits. +- `hotfix/` for urgent fixes based on `main`. + +Examples: + +- `feature/add-postgres-dialect` +- `fix/preserve-copy-options` +- `docs/branching-strategy` +- `chore/update-ci-python` +- `release/0.2.0` +- `hotfix/formatter-crash` + +## Normal Change Flow + +1. Update local `main` from `origin/main`. +2. Create a short-lived branch. +3. Keep commits focused and related to one change. +4. Open a draft pull request early if feedback or CI visibility is useful. +5. Run `scripts/check` locally before marking the pull request ready. +6. Merge only after required checks pass and the review expectation is met. +7. Delete the branch after merge. + +Prefer squash merges for small, focused pull requests so `main` stays readable. +Use a regular merge only when preserving multiple commits adds clear value. + +## Pull Request Rules + +Each pull request should include: + +- A short summary of the behavior or documentation change. +- Tests or a note explaining why tests were not needed. +- Any migration, release, or compatibility notes. +- Confirmation that `scripts/check` passed locally, unless CI is the only + practical validation path. + +Pull requests should stay small. If a branch grows into unrelated changes, split +it before review. + +## Releases + +Releases are cut from `main`. + +1. Merge release preparation changes through a pull request. +2. Tag the release commit on `main`. +3. Build and publish artifacts from the tagged commit. + +Do not release from feature branches. + +## Repository Protection + +Configure GitHub branch protection for `main`: + +- Require pull requests before merging. +- Require status checks to pass before merging. +- Require branches to be up to date before merging when GitHub can enforce it + without creating excessive noise. +- Restrict direct pushes to `main`. + +One approval can be optional if required checks are strong, but changes should +still go through a pull request rather than direct pushes. diff --git a/py_sql_cleaner/adapters/sqlglot_formatter.py b/py_sql_cleaner/adapters/sqlglot_formatter.py index 50d9f8d..bb10a6a 100644 --- a/py_sql_cleaner/adapters/sqlglot_formatter.py +++ b/py_sql_cleaner/adapters/sqlglot_formatter.py @@ -1,11 +1,27 @@ from __future__ import annotations from abc import ABC, abstractmethod +from collections.abc import Iterator import sqlglot +from sqlglot.dialects import Dialect +from sqlglot.tokens import Tokenizer, TokenType +from py_sql_cleaner.domain.config import DEFAULT_BACKEND, DEFAULT_DIALECT from py_sql_cleaner.domain.errors import FormatterError +SUPPORTED_DIALECTS = ("generic", "mysql", "postgres", "redshift") +REDSHIFT_EXPLICIT_DIALECT_KEYWORDS = ( + "COPY", + "UNLOAD", + "IAM_ROLE", + "DISTKEY", + "SORTKEY", + "DISTSTYLE", + "ENCODE", +) +REDSHIFT_PRESERVE_COMMANDS = ("COPY", "UNLOAD") + class FormatterBackend(ABC): @abstractmethod @@ -15,6 +31,13 @@ def format(self, sql: str, dialect: str) -> str: class SqlglotFormatter(FormatterBackend): def format(self, sql: str, dialect: str) -> str: + dialect = normalize_dialect(dialect) + if not dialect and _has_redshift_explicit_dialect_keyword(sql): + raise FormatterError("redshift-specific SQL requires --dialect redshift") + if _dialect_name(dialect) == "redshift" and _starts_with_command( + sql, REDSHIFT_PRESERVE_COMMANDS + ): + return sql.strip() try: expressions = sqlglot.parse(sql, read=dialect) formatted = ";\n\n".join(expr.sql(dialect=dialect, pretty=True) for expr in expressions) @@ -26,11 +49,59 @@ def format(self, sql: str, dialect: str) -> str: return formatted +def normalize_dialect(dialect: str) -> str: + normalized = dialect.strip().lower() + if normalized == "generic": + return "" + dialect_name = normalized.split(",", maxsplit=1)[0].strip() + if dialect_name not in SUPPORTED_DIALECTS: + supported = ", ".join(SUPPORTED_DIALECTS) + raise FormatterError(f"unsupported SQL dialect: {dialect}. Supported dialects: {supported}") + try: + Dialect.get_or_raise(normalized) + except Exception as exc: + raise FormatterError(f"unsupported SQL dialect: {dialect}. {exc}") from exc + return normalized + + +def _dialect_name(dialect: str) -> str: + return dialect.split(",", maxsplit=1)[0].strip().lower() + + +def _has_redshift_explicit_dialect_keyword(sql: str) -> bool: + return any( + token.text.upper() in REDSHIFT_EXPLICIT_DIALECT_KEYWORDS for token in _word_tokens(sql) + ) + + +def _starts_with_command(sql: str, commands: tuple[str, ...]) -> bool: + first_token = next(iter(_word_tokens(sql)), None) + return bool(first_token and first_token.text.upper() in commands) + + +def _word_tokens(sql: str) -> Iterator: + return ( + token + for token in Tokenizer().tokenize(sql) + if token.token_type not in {TokenType.IDENTIFIER, TokenType.STRING, TokenType.UNKNOWN} + and not _is_backtick_quoted_token(sql, token) + ) + + +def _is_backtick_quoted_token(sql: str, token) -> bool: + return ( + token.start > 0 + and token.end + 1 < len(sql) + and sql[token.start - 1] == "`" + and sql[token.end + 1] == "`" + ) + + def get_formatter_backend(backend: str) -> FormatterBackend: - if backend != "sqlglot": + if backend != DEFAULT_BACKEND: raise FormatterError(f"unsupported formatter backend: {backend}") return SqlglotFormatter() -def format_sql(sql: str, dialect: str = "redshift", backend: str = "sqlglot") -> str: +def format_sql(sql: str, dialect: str = DEFAULT_DIALECT, backend: str = DEFAULT_BACKEND) -> str: return get_formatter_backend(backend).format(sql.strip(), dialect=dialect) diff --git a/py_sql_cleaner/application/format_source.py b/py_sql_cleaner/application/format_source.py index 5a19d00..e179975 100644 --- a/py_sql_cleaner/application/format_source.py +++ b/py_sql_cleaner/application/format_source.py @@ -12,6 +12,7 @@ class FormatSourceResult: source: str warnings: list[str] + errors: list[str] def format_source( @@ -24,6 +25,7 @@ def format_source( formatter: SqlFormatter, ) -> FormatSourceResult: warnings: list[str] = [] + errors: list[str] = [] new_source = source for block in reversed(blocks): reason = unsafe_reason(block) @@ -35,14 +37,14 @@ def format_source( continue try: formatted = formatter(block.raw_sql, dialect, backend) - except FormatterError: - warnings.append( + except FormatterError as exc: + errors.append( f"failed to format SQL block {block.file_path}:{block.start_line}-" - f"{block.end_line} with {backend}. Skipping." + f"{block.end_line} with {backend}: {exc}. Skipping." ) continue new_source = replace_sql_content(new_source, block, formatted) - return FormatSourceResult(source=new_source, warnings=warnings) + return FormatSourceResult(source=new_source, warnings=warnings, errors=errors) def unsafe_reason(block: SqlBlock) -> str | None: diff --git a/py_sql_cleaner/cli/app.py b/py_sql_cleaner/cli/app.py index ef6d16d..a495e9c 100644 --- a/py_sql_cleaner/cli/app.py +++ b/py_sql_cleaner/cli/app.py @@ -7,10 +7,17 @@ import typer from rich.console import Console -from py_sql_cleaner.adapters.sqlglot_formatter import format_sql +from py_sql_cleaner.adapters.sqlglot_formatter import ( + SUPPORTED_DIALECTS, + format_sql, + get_formatter_backend, + normalize_dialect, +) from py_sql_cleaner.application.extract_sql import plan_extract from py_sql_cleaner.application.format_source import format_source, is_unsafe from py_sql_cleaner.core.detector import detect_sql_blocks +from py_sql_cleaner.domain.config import DEFAULT_BACKEND, DEFAULT_DIALECT +from py_sql_cleaner.domain.errors import FormatterError app = typer.Typer(no_args_is_help=True) console = Console() @@ -36,28 +43,44 @@ def list_blocks(file: Annotated[Path, typer.Argument(exists=True, dir_okay=False @app.command("format") def format_command( file: Annotated[Path, typer.Argument(exists=True, dir_okay=False)], - dialect: Annotated[str, typer.Option()] = "redshift", - backend: Annotated[str, typer.Option()] = "sqlglot", + dialect: Annotated[ + str, + typer.Option( + "-d", + "--dialect", + help="SQLGlot dialect to use for parsing and formatting. Defaults to generic SQL.", + ), + ] = DEFAULT_DIALECT, + backend: Annotated[str, typer.Option()] = DEFAULT_BACKEND, write: Annotated[bool, typer.Option("--write/--check")] = True, dry_run: Annotated[bool, typer.Option("--dry-run")] = False, include_unsafe: Annotated[bool, typer.Option("--include-unsafe")] = False, ) -> None: + _validate_formatter_options(dialect, backend) source = file.read_text(encoding="utf-8") blocks = detect_sql_blocks(file, source) if not blocks: console.print("No embedded SQL blocks found.") raise typer.Exit(0) - result = format_source( - source, - blocks, - dialect=dialect, - backend=backend, - include_unsafe=include_unsafe, - formatter=format_sql, - ) + try: + result = format_source( + source, + blocks, + dialect=dialect, + backend=backend, + include_unsafe=include_unsafe, + formatter=format_sql, + ) + except FormatterError as exc: + console.print(f"Error: {exc}") + raise typer.Exit(1) from exc for warning in result.warnings: console.print(f"Warning: {warning}") + for error in result.errors: + console.print(f"Error: {error}") + if result.errors: + raise typer.Exit(1) changed = result.source != source if dry_run: @@ -82,8 +105,15 @@ def format_command( @app.command("check") def check_command( file: Annotated[Path, typer.Argument(exists=True, dir_okay=False)], - dialect: Annotated[str, typer.Option()] = "redshift", - backend: Annotated[str, typer.Option()] = "sqlglot", + dialect: Annotated[ + str, + typer.Option( + "-d", + "--dialect", + help="SQLGlot dialect to use for parsing and formatting. Defaults to generic SQL.", + ), + ] = DEFAULT_DIALECT, + backend: Annotated[str, typer.Option()] = DEFAULT_BACKEND, ) -> None: format_command(file, dialect=dialect, backend=backend, write=False, dry_run=False) @@ -92,12 +122,20 @@ def check_command( def extract_command( file: Annotated[Path, typer.Argument(exists=True, dir_okay=False)], out_dir: Annotated[Path, typer.Option("--out-dir")] = Path("sql"), - dialect: Annotated[str, typer.Option()] = "redshift", - backend: Annotated[str, typer.Option()] = "sqlglot", + dialect: Annotated[ + str, + typer.Option( + "-d", + "--dialect", + help="SQLGlot dialect to use for extracted SQL. Defaults to generic SQL.", + ), + ] = DEFAULT_DIALECT, + backend: Annotated[str, typer.Option()] = DEFAULT_BACKEND, replace_mode: Annotated[str, typer.Option("--replace-mode")] = "path", dry_run: Annotated[bool, typer.Option("--dry-run")] = False, name: Annotated[str | None, typer.Option("--name")] = None, ) -> None: + _validate_formatter_options(dialect, backend) source = file.read_text(encoding="utf-8") blocks = detect_sql_blocks(file, source) if not blocks: @@ -107,17 +145,21 @@ def extract_command( console.print("Error: --name can only be used when exactly one SQL block is detected.") raise typer.Exit(1) - result = plan_extract( - file, - source, - blocks, - out_dir=out_dir, - dialect=dialect, - backend=backend, - replace_mode=replace_mode, - name=name, - formatter=format_sql, - ) + try: + result = plan_extract( + file, + source, + blocks, + out_dir=out_dir, + dialect=dialect, + backend=backend, + replace_mode=replace_mode, + name=name, + formatter=format_sql, + ) + except FormatterError as exc: + console.print(f"Error: {exc}") + raise typer.Exit(1) from exc for warning in result.warnings: console.print(f"Warning: {warning}") @@ -139,6 +181,13 @@ def extract_command( console.print("Warning: replace-mode=read-text requires `from pathlib import Path`.") +@app.command("dialects") +def dialects_command() -> None: + """List SQL dialect values accepted by --dialect.""" + for dialect in SUPPORTED_DIALECTS: + console.print(dialect) + + def _print_diff(file: Path, old: str, new: str) -> None: if old == new: console.print("No changes.") @@ -153,6 +202,15 @@ def _print_diff(file: Path, old: str, new: str) -> None: console.print("\n".join(diff)) +def _validate_formatter_options(dialect: str, backend: str) -> None: + try: + normalize_dialect(dialect) + get_formatter_backend(backend) + except FormatterError as exc: + console.print(f"Error: {exc}") + raise typer.Exit(1) from exc + + def _write_sql_file_at_path(path: Path, sql: str) -> None: path.parent.mkdir(parents=True, exist_ok=True) path.write_text(_ensure_trailing_newline(sql), encoding="utf-8") diff --git a/py_sql_cleaner/domain/config.py b/py_sql_cleaner/domain/config.py index 384d746..838e39e 100644 --- a/py_sql_cleaner/domain/config.py +++ b/py_sql_cleaner/domain/config.py @@ -1,3 +1,3 @@ -DEFAULT_DIALECT = "redshift" +DEFAULT_DIALECT = "generic" DEFAULT_BACKEND = "sqlglot" MIN_CONFIDENCE = 0.7 diff --git a/tests/README.md b/tests/README.md new file mode 100644 index 0000000..9c350c8 --- /dev/null +++ b/tests/README.md @@ -0,0 +1,11 @@ +# Test Organization + +Tests are grouped by the package boundary or user-facing surface they exercise: + +- `adapters/` tests external integrations such as SQLGlot formatting. +- `cli/` tests Typer command behavior and filesystem effects owned by commands. +- `core/` tests deterministic detection, extraction, and rewriting logic. +- `fixtures/` contains input files shared by tests. + +When adding tests, place them next to the responsibility being verified instead +of adding more files directly under `tests/`. diff --git a/tests/adapters/test_sqlglot_formatter.py b/tests/adapters/test_sqlglot_formatter.py new file mode 100644 index 0000000..feec63a --- /dev/null +++ b/tests/adapters/test_sqlglot_formatter.py @@ -0,0 +1,186 @@ +import pytest + +from py_sql_cleaner.adapters.sqlglot_formatter import format_sql +from py_sql_cleaner.domain.errors import FormatterError + + +def test_formats_with_clause() -> None: + sql = """ +with base as ( + select user_id, updated_at + from users +) +select * +from base; +""" + + formatted = format_sql(sql) + + assert "WITH" in formatted + assert "SELECT" in formatted + assert formatted.endswith(";") + + +def test_defaults_to_generic_sqlglot_dialect() -> None: + sql = """ +select * from users; +""" + + formatted = format_sql(sql) + + assert "SELECT" in formatted + assert "FROM users" in formatted + + +def test_formats_with_postgres_dialect() -> None: + sql = """ +select payload->>'name' as name +from events; +""" + + formatted = format_sql(sql, dialect="postgres") + + assert "payload ->> 'name'" in formatted + assert "FROM events" in formatted + + +def test_formats_with_mysql_dialect() -> None: + sql = """ +select `user_id` +from `users`; +""" + + formatted = format_sql(sql, dialect="mysql") + + assert "`user_id`" in formatted + assert "FROM `users`" in formatted + + +def test_formats_with_sqlglot_dialect_settings() -> None: + sql = """ +select * from users; +""" + + formatted = format_sql(sql, dialect="postgres, normalization_strategy=case_sensitive") + + assert "SELECT" in formatted + assert "FROM users" in formatted + + +def test_formats_qualify_without_removing_window_clause() -> None: + sql = """ +select user_id, updated_at +from users +qualify row_number() over(partition by user_id order by updated_at desc)=1; +""" + + formatted = format_sql(sql) + + assert "QUALIFY" in formatted + assert "ROW_NUMBER" in formatted + assert "PARTITION BY" in formatted + + +def test_formats_with_qualify() -> None: + sql = """ +with base as ( + select user_id, updated_at + from users +) +select * +from base +qualify row_number() over(partition by user_id order by updated_at desc)=1; +""" + + formatted = format_sql(sql) + + assert "WITH" in formatted + assert "QUALIFY" in formatted + assert "ROW_NUMBER" in formatted + + +def test_formats_copy_without_dropping_redshift_clauses() -> None: + sql = """ +COPY users +FROM '' +IAM_ROLE '' +FORMAT AS CSV +IGNOREHEADER 1; +""" + + formatted = format_sql(sql, dialect="redshift") + + assert "COPY users" in formatted + assert "IAM_ROLE" in formatted + assert "IGNOREHEADER 1" in formatted + + +def test_redshift_copy_command_is_preserved_without_rewriting_options() -> None: + sql = """COPY users +FROM '' +IAM_ROLE '' +FORMAT AS CSV +TIMEFORMAT 'auto' +IGNOREHEADER 1;""" + + formatted = format_sql(sql, dialect="redshift") + + assert formatted == sql + + +def test_redshift_specific_command_requires_explicit_redshift_dialect() -> None: + sql = """ +COPY users +FROM '' +IAM_ROLE '' +FORMAT AS CSV; +""" + + with pytest.raises(FormatterError, match="requires --dialect redshift"): + format_sql(sql) + + +@pytest.mark.parametrize( + "sql", + [ + "select copy_count from users;", + "select encoded_payload from events;", + "select 'COPY' as word from events;", + "select * from users where note = 'unload complete';", + 'select "COPY" from events;', + ], +) +def test_redshift_keyword_detection_ignores_identifiers_and_literals(sql: str) -> None: + formatted = format_sql(sql) + + assert "SELECT" in formatted + + +def test_unload_is_preserved_even_when_sqlglot_uses_command_fallback() -> None: + sql = """ +UNLOAD ('SELECT * FROM users') +TO '' +IAM_ROLE '' +FORMAT AS PARQUET; +""" + + formatted = format_sql(sql, dialect="redshift") + + assert "UNLOAD" in formatted + assert "TO ''" in formatted + assert "FORMAT AS PARQUET" in formatted + + +def test_unsupported_backend_raises_formatter_error() -> None: + with pytest.raises(FormatterError): + format_sql("select * from users", backend="unknown") + + +def test_unsupported_dialect_raises_formatter_error() -> None: + with pytest.raises(FormatterError, match="unsupported SQL dialect"): + format_sql("select * from users", dialect="unknown") + + +def test_unreviewed_sqlglot_dialect_raises_formatter_error() -> None: + with pytest.raises(FormatterError, match="generic, mysql, postgres, redshift"): + format_sql("select * from users", dialect="snowflake") diff --git a/tests/test_extract_command.py b/tests/cli/test_extract_command.py similarity index 57% rename from tests/test_extract_command.py rename to tests/cli/test_extract_command.py index 0aec191..0ef77ea 100644 --- a/tests/test_extract_command.py +++ b/tests/cli/test_extract_command.py @@ -24,6 +24,76 @@ def test_extract_writes_sql_file_and_replaces_python_string(tmp_path) -> None: assert file.read_text(encoding="utf-8") == 'query = "sql/query.sql"\n' +def test_extract_accepts_explicit_dialect(tmp_path) -> None: + file = tmp_path / "foo.py" + file.write_text( + '''query = """ +select payload->>'name' as name +from events +""" +''', + encoding="utf-8", + ) + + result = runner.invoke( + app, + ["extract", str(file), "--out-dir", "sql", "--dialect", "postgres"], + ) + + assert result.exit_code == 0, result.output + sql_file = tmp_path / "sql" / "query.sql" + assert "payload ->> 'name'" in sql_file.read_text(encoding="utf-8") + + +def test_extract_accepts_mysql_dialect(tmp_path) -> None: + file = tmp_path / "foo.py" + file.write_text( + '''query = """ +select `user_id` +from `users` +""" +''', + encoding="utf-8", + ) + + result = runner.invoke( + app, + ["extract", str(file), "--out-dir", "sql", "-d", "mysql"], + ) + + assert result.exit_code == 0, result.output + sql_file = tmp_path / "sql" / "query.sql" + assert "`user_id`" in sql_file.read_text(encoding="utf-8") + assert "FROM `users`" in sql_file.read_text(encoding="utf-8") + + +def test_extract_accepts_redshift_dialect(tmp_path) -> None: + file = tmp_path / "foo.py" + file.write_text( + '''query = """ +COPY users +FROM '' +IAM_ROLE '' +FORMAT AS CSV +IGNOREHEADER 1 +""" +''', + encoding="utf-8", + ) + + result = runner.invoke( + app, + ["extract", str(file), "--out-dir", "sql", "-d", "redshift"], + ) + + assert result.exit_code == 0, result.output + sql_file = tmp_path / "sql" / "query.sql" + sql = sql_file.read_text(encoding="utf-8") + assert "COPY users" in sql + assert "IAM_ROLE" in sql + assert "IGNOREHEADER 1" in sql + + def test_extract_dry_run_does_not_modify_files(tmp_path) -> None: file = tmp_path / "foo.py" original = '''query = """ diff --git a/tests/cli/test_format_command.py b/tests/cli/test_format_command.py new file mode 100644 index 0000000..9e0f54c --- /dev/null +++ b/tests/cli/test_format_command.py @@ -0,0 +1,221 @@ +from typer.testing import CliRunner + +from py_sql_cleaner.cli import app + +runner = CliRunner() + + +def test_format_command_rewrites_embedded_sql(tmp_path) -> None: + file = tmp_path / "foo.py" + file.write_text( + '''query = """ +select user_id, updated_at +from users +qualify row_number() over(partition by user_id order by updated_at desc)=1 +""" +''', + encoding="utf-8", + ) + + result = runner.invoke(app, ["format", str(file)]) + + assert result.exit_code == 0, result.output + source = file.read_text(encoding="utf-8") + assert "SELECT" in source + assert "QUALIFY" in source + assert "ROW_NUMBER" in source + assert "PARTITION BY" in source + + +def test_format_command_accepts_explicit_dialect(tmp_path) -> None: + file = tmp_path / "foo.py" + file.write_text( + '''query = """ +select payload->>'name' as name +from events +""" +''', + encoding="utf-8", + ) + + result = runner.invoke(app, ["format", str(file), "--dialect", "postgres"]) + + assert result.exit_code == 0, result.output + source = file.read_text(encoding="utf-8") + assert "payload ->> 'name'" in source + assert "FROM events" in source + + +def test_format_command_preserves_redshift_copy_with_explicit_dialect(tmp_path) -> None: + file = tmp_path / "foo.py" + file.write_text( + '''query = """ +COPY users +FROM '' +IAM_ROLE '' +FORMAT AS CSV +IGNOREHEADER 1 +""" +''', + encoding="utf-8", + ) + + result = runner.invoke(app, ["format", str(file), "-d", "redshift"]) + + assert result.exit_code == 0, result.output + source = file.read_text(encoding="utf-8") + assert "COPY users" in source + assert "IAM_ROLE" in source + assert "IGNOREHEADER 1" in source + + +def test_format_command_rejects_redshift_copy_without_explicit_dialect(tmp_path) -> None: + file = tmp_path / "foo.py" + original = '''query = """ +COPY users +FROM '' +IAM_ROLE '' +FORMAT AS CSV +IGNOREHEADER 1 +""" +''' + file.write_text(original, encoding="utf-8") + + result = runner.invoke(app, ["format", str(file)]) + + assert result.exit_code == 1 + assert "requires --dialect redshift" in result.output + assert file.read_text(encoding="utf-8") == original + + +def test_check_returns_one_for_redshift_copy_without_explicit_dialect(tmp_path) -> None: + file = tmp_path / "foo.py" + file.write_text( + '''query = """ +COPY users +FROM '' +IAM_ROLE '' +FORMAT AS CSV +IGNOREHEADER 1 +""" +''', + encoding="utf-8", + ) + + result = runner.invoke(app, ["check", str(file)]) + + assert result.exit_code == 1 + assert "requires --dialect redshift" in result.output + + +def test_format_command_preserves_mysql_backticks_with_explicit_dialect(tmp_path) -> None: + file = tmp_path / "foo.py" + file.write_text( + '''query = """ +select `user_id` +from `users` +""" +''', + encoding="utf-8", + ) + + result = runner.invoke(app, ["format", str(file), "-d", "mysql"]) + + assert result.exit_code == 0, result.output + source = file.read_text(encoding="utf-8") + assert "`user_id`" in source + assert "FROM `users`" in source + + +def test_format_command_accepts_short_dialect_option(tmp_path) -> None: + file = tmp_path / "foo.py" + file.write_text( + '''query = """ +select payload->>'name' as name +from events +""" +''', + encoding="utf-8", + ) + + result = runner.invoke(app, ["format", str(file), "-d", "postgres"]) + + assert result.exit_code == 0, result.output + assert "payload ->> 'name'" in file.read_text(encoding="utf-8") + + +def test_format_command_rejects_unknown_dialect(tmp_path) -> None: + file = tmp_path / "foo.py" + file.write_text( + '''query = """ +select * from users +""" +''', + encoding="utf-8", + ) + + result = runner.invoke(app, ["format", str(file), "--dialect", "unknown"]) + + assert result.exit_code == 1 + assert "unsupported SQL dialect" in result.output + assert "generic" in result.output + assert "mysql" in result.output + assert "postgres" in result.output + assert "redshift" in result.output + + +def test_dialects_command_lists_supported_dialects() -> None: + result = runner.invoke(app, ["dialects"]) + + assert result.exit_code == 0, result.output + assert "generic" in result.output + assert "mysql" in result.output + assert "postgres" in result.output + assert "redshift" in result.output + assert "snowflake" not in result.output + + +def test_format_dry_run_does_not_modify_file(tmp_path) -> None: + file = tmp_path / "foo.py" + original = '''query = """ +select * from users +""" +''' + file.write_text(original, encoding="utf-8") + + result = runner.invoke(app, ["format", str(file), "--dry-run"]) + + assert result.exit_code == 0, result.output + assert file.read_text(encoding="utf-8") == original + assert "SELECT" in result.output + + +def test_check_returns_one_for_unformatted_sql(tmp_path) -> None: + file = tmp_path / "foo.py" + file.write_text( + '''query = """ +select * from users +""" +''', + encoding="utf-8", + ) + + result = runner.invoke(app, ["check", str(file)]) + + assert result.exit_code == 1 + assert "Found unformatted embedded SQL" in result.output + + +def test_format_skips_jinja_sql(tmp_path) -> None: + file = tmp_path / "foo.py" + original = '''query = """ +select * from users where ds = '{{ ds }}' +""" +''' + file.write_text(original, encoding="utf-8") + + result = runner.invoke(app, ["format", str(file)]) + + assert result.exit_code == 0, result.output + assert file.read_text(encoding="utf-8") == original + assert "reason=jinja" in result.output diff --git a/tests/test_list_command.py b/tests/cli/test_list_command.py similarity index 100% rename from tests/test_list_command.py rename to tests/cli/test_list_command.py diff --git a/tests/test_detector.py b/tests/core/test_detector.py similarity index 66% rename from tests/test_detector.py rename to tests/core/test_detector.py index 6999844..56ae46f 100644 --- a/tests/test_detector.py +++ b/tests/core/test_detector.py @@ -63,3 +63,42 @@ def test_marks_jinja_like_sql() -> None: assert len(blocks) == 1 assert blocks[0].has_jinja is True + + +def test_detects_redshift_copy_statement() -> None: + source = '''query = """ +COPY users +FROM '' +IAM_ROLE '' +FORMAT AS CSV +""" +''' + + blocks = detect_sql_blocks(Path("foo.py"), source) + + assert len(blocks) == 1 + assert blocks[0].confidence == 0.95 + + +def test_detects_postgres_operator_statement() -> None: + source = '''query = """ +select payload->>'name' as name +from events +""" +''' + + blocks = detect_sql_blocks(Path("foo.py"), source) + + assert len(blocks) == 1 + + +def test_detects_mysql_backtick_statement() -> None: + source = '''query = """ +select `user_id` +from `users` +""" +''' + + blocks = detect_sql_blocks(Path("foo.py"), source) + + assert len(blocks) == 1 diff --git a/tests/test_format_command.py b/tests/test_format_command.py deleted file mode 100644 index 39f6273..0000000 --- a/tests/test_format_command.py +++ /dev/null @@ -1,73 +0,0 @@ -from typer.testing import CliRunner - -from py_sql_cleaner.cli import app - -runner = CliRunner() - - -def test_format_command_rewrites_embedded_sql(tmp_path) -> None: - file = tmp_path / "foo.py" - file.write_text( - '''query = """ -select user_id, updated_at -from users -qualify row_number() over(partition by user_id order by updated_at desc)=1 -""" -''', - encoding="utf-8", - ) - - result = runner.invoke(app, ["format", str(file)]) - - assert result.exit_code == 0, result.output - source = file.read_text(encoding="utf-8") - assert "SELECT" in source - assert "QUALIFY" in source - assert "ROW_NUMBER" in source - assert "PARTITION BY" in source - - -def test_format_dry_run_does_not_modify_file(tmp_path) -> None: - file = tmp_path / "foo.py" - original = '''query = """ -select * from users -""" -''' - file.write_text(original, encoding="utf-8") - - result = runner.invoke(app, ["format", str(file), "--dry-run"]) - - assert result.exit_code == 0, result.output - assert file.read_text(encoding="utf-8") == original - assert "SELECT" in result.output - - -def test_check_returns_one_for_unformatted_sql(tmp_path) -> None: - file = tmp_path / "foo.py" - file.write_text( - '''query = """ -select * from users -""" -''', - encoding="utf-8", - ) - - result = runner.invoke(app, ["check", str(file)]) - - assert result.exit_code == 1 - assert "Found unformatted embedded SQL" in result.output - - -def test_format_skips_jinja_sql(tmp_path) -> None: - file = tmp_path / "foo.py" - original = '''query = """ -select * from users where ds = '{{ ds }}' -""" -''' - file.write_text(original, encoding="utf-8") - - result = runner.invoke(app, ["format", str(file)]) - - assert result.exit_code == 0, result.output - assert file.read_text(encoding="utf-8") == original - assert "reason=jinja" in result.output diff --git a/tests/test_formatter.py b/tests/test_formatter.py deleted file mode 100644 index a9099f7..0000000 --- a/tests/test_formatter.py +++ /dev/null @@ -1,89 +0,0 @@ -import pytest - -from py_sql_cleaner.adapters.sqlglot_formatter import format_sql -from py_sql_cleaner.domain.errors import FormatterError - - -def test_formats_with_clause() -> None: - sql = """ -with base as ( - select user_id, updated_at - from users -) -select * -from base; -""" - - formatted = format_sql(sql) - - assert "WITH" in formatted - assert "SELECT" in formatted - assert formatted.endswith(";") - - -def test_formats_qualify_without_removing_window_clause() -> None: - sql = """ -select user_id, updated_at -from users -qualify row_number() over(partition by user_id order by updated_at desc)=1; -""" - - formatted = format_sql(sql) - - assert "QUALIFY" in formatted - assert "ROW_NUMBER" in formatted - assert "PARTITION BY" in formatted - - -def test_formats_with_qualify() -> None: - sql = """ -with base as ( - select user_id, updated_at - from users -) -select * -from base -qualify row_number() over(partition by user_id order by updated_at desc)=1; -""" - - formatted = format_sql(sql) - - assert "WITH" in formatted - assert "QUALIFY" in formatted - assert "ROW_NUMBER" in formatted - - -def test_formats_copy_without_dropping_redshift_clauses() -> None: - sql = """ -COPY users -FROM '' -IAM_ROLE '' -FORMAT AS CSV -IGNOREHEADER 1; -""" - - formatted = format_sql(sql) - - assert "COPY users" in formatted - assert "IAM_ROLE" in formatted - assert "IGNOREHEADER 1" in formatted - - -def test_unload_is_preserved_even_when_sqlglot_uses_command_fallback() -> None: - sql = """ -UNLOAD ('SELECT * FROM users') -TO '' -IAM_ROLE '' -FORMAT AS PARQUET; -""" - - formatted = format_sql(sql) - - assert "UNLOAD" in formatted - assert "TO ''" in formatted - assert "FORMAT AS PARQUET" in formatted - - -def test_unsupported_backend_raises_formatter_error() -> None: - with pytest.raises(FormatterError): - format_sql("select * from users", backend="unknown") diff --git a/website/docs/intro.md b/website/docs/intro.md index 1a730ca..c4fd89f 100644 --- a/website/docs/intro.md +++ b/website/docs/intro.md @@ -8,8 +8,9 @@ sidebar_position: 1 embedded in Python files. It is built for codebases where long SQL queries are written directly inside -triple-quoted Python strings. The current MVP defaults to the Redshift dialect -through SQLGlot, with room for broader dialect support over time. +triple-quoted Python strings. The current MVP uses SQLGlot for formatting, +defaults to SQLGlot's generic dialect, and can format with database-specific +dialects that this project has explicitly enabled via `--dialect`. ```python query = """ @@ -25,6 +26,10 @@ file. :::note `py-sql-cleaner` is an early MVP. It uses SQLGlot internally for best-effort SQL formatting. It does not connect to databases and does not execute SQL. +Dialect support means SQLGlot parser/formatter mode selection, not exhaustive +database validation. Redshift command-style statements such as `COPY` and +`UNLOAD` are preserved rather than reformatted to avoid changing load/export +options. ::: ## What It Helps With @@ -38,6 +43,7 @@ formatting. It does not connect to databases and does not execute SQL. - Python files - triple-quoted SQL strings -- SQLGlot-backed SQL formatting, currently defaulting to the Redshift dialect +- SQLGlot-backed SQL formatting, defaulting to generic SQL with `--dialect` + support for explicitly enabled database-specific formatting - formatting - extracting SQL into `.sql` files diff --git a/website/docs/project/contributing.md b/website/docs/project/contributing.md index 292bd76..669eebf 100644 --- a/website/docs/project/contributing.md +++ b/website/docs/project/contributing.md @@ -31,6 +31,11 @@ Check built distributions: .venv/bin/python -m twine check dist/* ``` +## Code Review Tooling + +CodeRabbit repository settings live in `.coderabbit.yaml`. Install the CodeRabbit +GitHub App on this public repository to enable automated pull request reviews. + ## Guidelines - Keep changes focused. diff --git a/website/docs/project/status.md b/website/docs/project/status.md index 655c365..9ed349e 100644 --- a/website/docs/project/status.md +++ b/website/docs/project/status.md @@ -10,13 +10,15 @@ The current focus is: - Python files - triple-quoted SQL strings -- SQLGlot-backed SQL formatting, currently defaulting to the Redshift dialect +- SQLGlot-backed SQL formatting, defaulting to generic SQL with `--dialect` + support for explicitly enabled database-specific formatting - formatting - extracting SQL into `.sql` files Future versions may add: -- PostgreSQL support +- additional SQL dialects based on user demand +- database-specific validation beyond SQLGlot formatting - better template handling - safer f-string handling - pre-commit integration diff --git a/website/docs/reference/commands.md b/website/docs/reference/commands.md index 1bb193f..69519d6 100644 --- a/website/docs/reference/commands.md +++ b/website/docs/reference/commands.md @@ -10,6 +10,7 @@ sidebar_position: 1 | `format` | Format embedded SQL in place | `py-sql-cleaner format jobs/load_users.py` | | `check` | Check whether embedded SQL is formatted | `py-sql-cleaner check jobs/load_users.py` | | `extract` | Extract embedded SQL into `.sql` files | `py-sql-cleaner extract jobs/load_users.py --out-dir sql` | +| `dialects` | List accepted dialect values | `py-sql-cleaner dialects` | ## `list` @@ -23,6 +24,16 @@ py-sql-cleaner list path/to/file.py py-sql-cleaner format path/to/file.py ``` +Use a database-specific SQLGlot dialect: + +```bash +py-sql-cleaner format path/to/file.py -d redshift +``` + +Currently enabled dialects are `generic`, `mysql`, `postgres`, and `redshift`. +Dialect support is best-effort formatting through SQLGlot. Redshift command-style +statements such as `COPY` and `UNLOAD` are preserved rather than reformatted. + Preview changes: ```bash @@ -35,12 +46,24 @@ py-sql-cleaner format path/to/file.py --dry-run py-sql-cleaner check path/to/file.py ``` +Use the same dialect option as `format`: + +```bash +py-sql-cleaner check path/to/file.py -d postgres +``` + ## `extract` ```bash py-sql-cleaner extract path/to/file.py --out-dir sql ``` +Format extracted SQL with a database-specific dialect: + +```bash +py-sql-cleaner extract path/to/file.py --out-dir sql -d postgres +``` + Use `read-text` replacement mode: ```bash @@ -63,3 +86,11 @@ Specify an output name: ```bash py-sql-cleaner extract path/to/file.py --out-dir sql --name load_users ``` + +## `dialects` + +```bash +py-sql-cleaner dialects +``` + +The output lists the dialect values accepted by `--dialect` / `-d`. diff --git a/website/docs/reference/supported-input.md b/website/docs/reference/supported-input.md index 7c06098..e9f3f2f 100644 --- a/website/docs/reference/supported-input.md +++ b/website/docs/reference/supported-input.md @@ -42,6 +42,9 @@ Detected variable names include: - `CREATE` - `MERGE` +Redshift command-style statements such as `COPY` and `UNLOAD` should be run with +`-d redshift`. They are detected and preserved rather than reformatted. + ## Not Targeted in the MVP ```python diff --git a/website/src/pages/index.module.css b/website/src/pages/index.module.css index 4b28328..a177e7c 100644 --- a/website/src/pages/index.module.css +++ b/website/src/pages/index.module.css @@ -29,8 +29,8 @@ .heroGrid { position: relative; display: grid; - gap: 3rem; - grid-template-columns: minmax(0, 0.84fr) minmax(440px, 1.16fr); + gap: 6.4rem; + grid-template-columns: minmax(0, 0.92fr) minmax(420px, 1.08fr); align-items: center; }