-
Notifications
You must be signed in to change notification settings - Fork 260
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", | ||||||||||||||
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. 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.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback 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. 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? |
||||||||||||||
) | ||||||||||||||
with open(model_path, "w", encoding="utf-8") as f: | ||||||||||||||
f.write(content) | ||||||||||||||
|
||||||||||||||
result = runner.invoke(cli, ["--paths", tmp_path, "lint", "--fix"]) | ||||||||||||||
assert result.exit_code == 0 | ||||||||||||||
with open(model_path, "r", encoding="utf-8") as f: | ||||||||||||||
assert "SELECT *" not in f.read() | ||||||||||||||
Comment on lines
+1354
to
+1355
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. Should this check for the exact expected output instead of this negation? |
||||||||||||||
|
||||||||||||||
|
||||||||||||||
def test_lint_fix_unfixable_error(runner, tmp_path): | ||||||||||||||
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. Should we notify users when a lint error is unfixable? |
||||||||||||||
create_example_project(tmp_path) | ||||||||||||||
|
||||||||||||||
with open(tmp_path / "config.yaml", "a", encoding="utf-8") as f: | ||||||||||||||
f.write( | ||||||||||||||
"""linter: | ||||||||||||||
enabled: True | ||||||||||||||
rules: ["noselectstar", "nomissingaudits"] | ||||||||||||||
""" | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
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", | ||||||||||||||
) | ||||||||||||||
with open(model_path, "w", encoding="utf-8") as f: | ||||||||||||||
f.write(content) | ||||||||||||||
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. This string replacement pattern is duplicated from the previous test. Consider extracting this into a helper function to reduce code duplication and improve maintainability.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||||||||
|
||||||||||||||
result = runner.invoke(cli, ["--paths", tmp_path, "lint", "--fix"]) | ||||||||||||||
assert result.exit_code == 1 | ||||||||||||||
assert "nomissingaudits" in result.output | ||||||||||||||
with open(model_path, "r", encoding="utf-8") as f: | ||||||||||||||
assert "SELECT *" not in f.read() | ||||||||||||||
|
||||||||||||||
|
||||||||||||||
def test_state_export(runner: CliRunner, tmp_path: Path) -> None: | ||||||||||||||
create_example_project(tmp_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-fix
astral-sh/ruff#8191Just my 2c as a user - this isn't a review