Skip to content

Commit 8cd5ca5

Browse files
committed
fix(lint): address review feedback on path selection
The two assertions on the linted file name compared against raw console output. The console wraps at 80 columns, so a long enough temporary path splits the name across lines and the assertion fails. Under xdist the path gains a popen-gwN segment, which is what makes it reproducible there. Reproduced directly: a path of the right length renders the line as "models/seed_model.sq\nl:". Both assertions now compare against the output with the wrapping removed. A directory now says so instead of reporting that no models were found in it, which read as though the directory were empty. The docstring records that relative paths resolve against the current working directory rather than the project path. That is what path-based tools pass, but it means the Python API only accepts relative paths when called from inside the project. The linter guide now frames path selection as the option for tools that hand over file names, and points CI use towards a model selector instead of paths from git diff, so that the models affected by a change are linted rather than only the files that were edited. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
1 parent e23151c commit 8cd5ca5

3 files changed

Lines changed: 28 additions & 6 deletions

File tree

‎docs/guides/linter.md‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -127,9 +127,13 @@ Error: Linter detected errors in the code. Please fix them before proceeding.
127127

128128
Use `sqlmesh lint --help` for more information.
129129

130-
Models can be selected by name with `--model`, by model file path, or by both at once. Selecting by
131-
path lets `sqlmesh lint` be wired up to path-based tooling such as [pre-commit](https://pre-commit.com/),
132-
which passes the names of the changed files:
130+
Models can be selected by name with `--model`, by model file path, or by both at once.
131+
132+
Selecting by path is intended for tools that hand `sqlmesh lint` a list of file names, such as
133+
[pre-commit](https://pre-commit.com/) hooks and editor integrations. To lint the models that changed
134+
on a branch in CI, prefer a model selector such as `git:main` over passing paths from
135+
`git diff --name-only`, so that the models affected by a change are selected rather than only the
136+
files that were edited.
133137

134138
``` bash
135139
$ sqlmesh lint models/full_model.sql models/incremental_model.sql

‎sqlmesh/core/context.py‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3617,12 +3617,22 @@ def lint_models(
36173617
value of ``linter.use_project_index`` is used. Indexed linting of selected
36183618
models reloads an already-loaded context so the requested scope is applied.
36193619
paths: Model file paths to lint, each resolved to the model(s) defined in it. Can be
3620-
combined with `models`.
3620+
combined with `models`. Relative paths are resolved against the current working
3621+
directory rather than the project path, which is what path-based tools such as
3622+
pre-commit pass. Calling this from the Python API with relative paths therefore
3623+
only works from inside the project directory.
36213624
"""
36223625
models = list(models) if models is not None else []
36233626
target_paths = [Path(path) for path in paths] if paths is not None else []
36243627

36253628
# Fail fast on a mistyped path instead of loading and linting the whole project.
3629+
directory_paths = [str(path) for path in target_paths if path.is_dir()]
3630+
if directory_paths:
3631+
raise SQLMeshError(
3632+
f"Expected model files but got director{'ies' if len(directory_paths) > 1 else 'y'}: "
3633+
f"{', '.join(directory_paths)}. Pass the model files themselves."
3634+
)
3635+
36263636
missing_paths = [str(path) for path in target_paths if not path.is_file()]
36273637
if missing_paths:
36283638
raise SQLMeshError(

‎tests/cli/test_cli.py‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1492,7 +1492,7 @@ def test_lint_paths(runner, tmp_path):
14921492
cli, ["--paths", tmp_path, "lint", str(tmp_path / "models" / "seed_model.sql")]
14931493
)
14941494
assert result.output.count("Linter errors for") == 1
1495-
assert "seed_model.sql" in result.output
1495+
assert "seed_model.sql" in result.output.replace("\n", "")
14961496
assert result.exit_code == 1
14971497

14981498
# Multiple model files can be passed, as pre-commit does.
@@ -1556,7 +1556,7 @@ def test_lint_relative_path(runner, tmp_path, monkeypatch):
15561556
monkeypatch.chdir(tmp_path)
15571557
result = runner.invoke(cli, ["--paths", tmp_path, "lint", "models/seed_model.sql"])
15581558
assert result.output.count("Linter errors for") == 1
1559-
assert "seed_model.sql" in result.output
1559+
assert "seed_model.sql" in result.output.replace("\n", "")
15601560
assert result.exit_code == 1
15611561

15621562

@@ -1586,6 +1586,14 @@ def test_lint_unknown_path(runner, tmp_path):
15861586
assert "No models were found at the following path(s)" in result.output
15871587
assert "Linter errors for" not in result.output
15881588

1589+
# A directory says so, rather than claiming it contains no models.
1590+
result = runner.invoke(cli, ["--paths", tmp_path, "lint", str(tmp_path / "models")])
1591+
assert result.exit_code == 1
1592+
output = result.output.replace("\n", "")
1593+
assert "Expected model files but got directory" in output
1594+
assert "Pass the model files themselves." in output
1595+
assert "Linter errors for" not in result.output
1596+
15891597

15901598
def test_lint_no_models(runner, tmp_path):
15911599
with open(tmp_path / "config.yaml", "w", encoding="utf-8") as f:

0 commit comments

Comments
 (0)