-
Notifications
You must be signed in to change notification settings - Fork 311
feat: add fix option to lint command #5121
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,6 +78,7 @@ | |
| from sqlmesh.core.environment import Environment, EnvironmentNamingInfo, EnvironmentStatements | ||
| from sqlmesh.core.loader import Loader | ||
| from sqlmesh.core.linter.definition import AnnotatedRuleViolation, Linter | ||
| from sqlmesh.core.linter.rule import TextEdit, Position | ||
| from sqlmesh.core.linter.rules import BUILTIN_RULES | ||
| from sqlmesh.core.macros import ExecutableOrMacro, macro | ||
| from sqlmesh.core.metric import Metric, rewrite | ||
|
|
@@ -3099,6 +3100,7 @@ def lint_models( | |
| self, | ||
| models: t.Optional[t.Iterable[t.Union[str, Model]]] = None, | ||
| raise_on_error: bool = True, | ||
| fix: bool = False, | ||
| ) -> t.List[AnnotatedRuleViolation]: | ||
| found_error = False | ||
|
|
||
|
|
@@ -3116,13 +3118,45 @@ def lint_models( | |
| found_error = True | ||
| all_violations.extend(violations) | ||
|
|
||
| if fix: | ||
| self._apply_fixes(all_violations) | ||
| self.refresh() | ||
| return self.lint_models(models, raise_on_error=raise_on_error, fix=False) | ||
|
|
||
| if raise_on_error and found_error: | ||
| raise LinterError( | ||
| "Linter detected errors in the code. Please fix them before proceeding." | ||
| ) | ||
|
|
||
| return all_violations | ||
|
|
||
| def _apply_fixes(self, violations: t.List[AnnotatedRuleViolation]) -> None: | ||
| edits_by_file: t.Dict[Path, t.List[TextEdit]] = {} | ||
| for violation in violations: | ||
| for fix in violation.fixes: | ||
| for create in fix.create_files: | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can there be conflicting edits? How do we handle them? |
||
|
|
||
| for path, edits in edits_by_file.items(): | ||
| content = path.read_text(encoding="utf-8") | ||
| lines = content.splitlines(keepends=True) | ||
|
|
||
| def _offset(pos: Position) -> int: | ||
| return sum(len(lines[i]) for i in range(pos.line)) + pos.character | ||
|
|
||
| for edit in sorted( | ||
| edits, key=lambda e: (e.range.start.line, e.range.start.character), reverse=True | ||
| ): | ||
| start = _offset(edit.range.start) | ||
| end = _offset(edit.range.end) | ||
| content = content[:start] + edit.new_text + content[end:] | ||
| lines = content.splitlines(keepends=True) | ||
|
|
||
| path.write_text(content, encoding="utf-8") | ||
|
|
||
| def load_model_tests( | ||
| self, tests: t.Optional[t.List[str]] = None, patterns: list[str] | None = None | ||
| ) -> t.List[ModelTestMetadata]: | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1328,6 +1328,61 @@ def test_lint(runner, tmp_path): | |||||||||||||
| assert result.exit_code == 1 | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def test_lint_fix(runner, tmp_path): | ||||||||||||||
| create_example_project(tmp_path) | ||||||||||||||
|
|
||||||||||||||
| with open(tmp_path / "config.yaml", "a", encoding="utf-8") as f: | ||||||||||||||
| f.write( | ||||||||||||||
| """linter: | ||||||||||||||
| enabled: True | ||||||||||||||
| rules: ["noselectstar"] | ||||||||||||||
| """ | ||||||||||||||
| ) | ||||||||||||||
|
|
||||||||||||||
| model_path = tmp_path / "models" / "incremental_model.sql" | ||||||||||||||
| with open(model_path, "r", encoding="utf-8") as f: | ||||||||||||||
| content = f.read() | ||||||||||||||
| content = content.replace( | ||||||||||||||
| "SELECT\n id,\n item_id,\n event_date,\nFROM", | ||||||||||||||
| "SELECT *\nFROM", | ||||||||||||||
|
||||||||||||||
| "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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this check for the exact expected output instead of this negation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we notify users when a lint error is unfixable?
Copilot
AI
Aug 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-fixastral-sh/ruff#8191Just my 2c as a user - this isn't a review