Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a --fix flag to the sqlmesh lint command that automatically applies available fixes for lint violations and fails if errors remain after fixes are applied.
- Added
--fixflag to the lint CLI command - Implemented fix application logic in the context's
lint_modelsmethod - Added comprehensive tests for both successful fixes and unfixable errors
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sqlmesh/cli/main.py | Added --fix flag option to lint command |
| sqlmesh/core/context.py | Implemented fix application logic and recursive linting after fixes |
| tests/cli/test_cli.py | Added test cases for fix functionality and unfixable error handling |
| docs/reference/cli.md | Updated CLI documentation to include the new --fix option |
| docs/guides/linter.md | Updated linter guide to mention the fix functionality |
| docs/examples/sqlmesh_cli_crash_course.md | Added example usage of --fix flag |
| content = f.read() | ||
| content = content.replace( | ||
| "SELECT\n id,\n item_id,\n event_date,\nFROM", | ||
| "SELECT *\nFROM", |
There was a problem hiding this comment.
The string replacement pattern is fragile and depends on exact whitespace matching. Consider using a more robust approach like regex replacement or a more specific pattern that's less likely to break with formatting changes.
| "SELECT *\nFROM", | |
| content = re.sub( | |
| r"SELECT\s*id,\s*item_id,\s*event_date,\s*FROM", | |
| "SELECT *\nFROM", | |
| content, | |
| flags=re.MULTILINE, |
There was a problem hiding this comment.
I think this is a valid comment. Can we just overwrite the file, or even better, create a new one to avoid assuming a certain structure?
| "SELECT *\nFROM", | ||
| ) | ||
| with open(model_path, "w", encoding="utf-8") as f: | ||
| f.write(content) |
There was a problem hiding this comment.
This string replacement pattern is duplicated from the previous test. Consider extracting this into a helper function to reduce code duplication and improve maintainability.
| f.write(content) | |
| replace_select_columns_with_star(model_path) |
|
|
||
| Options: | ||
| --model TEXT A model to lint. Multiple models can be linted. If no models are specified, every model will be linted. | ||
| --fix Apply fixes for lint errors. Fails if errors remain after fixes are applied. |
There was a problem hiding this comment.
Would be nice to fix and still exit non-zero, makes it easier to use the same script to both fix things locally and error in CI, without needing an extra layer of pre-commit or something to detect if files were changed.
Similar discussion over at ruff, where they ended up introducing --exit-non-zero-on-fix astral-sh/ruff#8191
Just my 2c as a user - this isn't a review
georgesittas
left a comment
georgesittas
left a comment
There was a problem hiding this comment.
Seems like a reasonable addition, despite being unfamiliar with TextEdits.
| create.path.parent.mkdir(parents=True, exist_ok=True) | ||
| create.path.write_text(create.text, encoding="utf-8") | ||
| for edit in fix.edits: | ||
| edits_by_file.setdefault(edit.path, []).append(edit) |
There was a problem hiding this comment.
Can there be conflicting edits? How do we handle them?
| with open(model_path, "r", encoding="utf-8") as f: | ||
| assert "SELECT *" not in f.read() |
There was a problem hiding this comment.
Should this check for the exact expected output instead of this negation?
| assert "SELECT *" not in f.read() | ||
|
|
||
|
|
||
| def test_lint_fix_unfixable_error(runner, tmp_path): |
There was a problem hiding this comment.
Should we notify users when a lint error is unfixable?
Summary
--fixflag tosqlmesh lintto apply automatic fixes and fail on remaining errorsTesting
pre-commit run --files docs/examples/sqlmesh_cli_crash_course.md docs/guides/linter.md docs/reference/cli.md sqlmesh/cli/main.py sqlmesh/core/context.py tests/cli/test_cli.pypytest tests/cli/test_cli.py::test_lint_fix tests/cli/test_cli.py::test_lint_fix_unfixable_error -qhttps://chatgpt.com/codex/tasks/task_e_6899c89c18fc8330a9329492edcfeaff