Skip to content

Fix!: Move linting from load to plan creation#4032

Merged
VaggelisD merged 3 commits intomainfrom
vaggelisd/lint_plan_time
Mar 26, 2025
Merged

Fix!: Move linting from load to plan creation#4032
VaggelisD merged 3 commits intomainfrom
vaggelisd/lint_plan_time

Conversation

@VaggelisD
Copy link
Contributor

Currently, linting happens during load time which has proven to cause the following issues:

  • It's triggered on every load (perf. overhead) and on commands such as sqlmesh info which don't always warrant linting
  • It's blocking the implementation of a sqlmesh lint command, since the Context is already loaded (and linted) before the CLI command has been registered
  • It's output is hidden from CI/CD since only the plan errors are shown, meaning that if LinterError is raised the CI/CD bot does not have the full output.

This PR moves the linting process to plan time which solves the aforementioned issues.

@VaggelisD VaggelisD force-pushed the vaggelisd/lint_plan_time branch from 94f823e to 4761a85 Compare March 25, 2025 22:04
@VaggelisD VaggelisD requested a review from a team March 25, 2025 22:25
Copy link
Contributor

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imo it makes sense to add a --skip-lints flag to the plan command, similar to --skip-tests, because you may just wanna iterate in dev and not have the linter yell at you.

LGTM, otherwise 👍

@VaggelisD
Copy link
Contributor Author

Imo it makes sense to add a --skip-lints flag to the plan command, similar to --skip-tests, because you may just wanna iterate in dev and not have the linter yell at you.

Added it in the latest commit

@VaggelisD VaggelisD requested a review from a team March 26, 2025 16:21
@VaggelisD VaggelisD merged commit 699c93f into main Mar 26, 2025
22 checks passed
@VaggelisD VaggelisD deleted the vaggelisd/lint_plan_time branch March 26, 2025 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants