-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add sqlfluff to pre-commit #11010
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: master
Are you sure you want to change the base?
Add sqlfluff to pre-commit #11010
Conversation
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.
Pull Request Overview
This PR integrates SQLFluff into the repository’s pre-commit setup to catch SQL syntax issues early by:
- Adding a
.sqlfluff
configuration file for Postgres dialect and style rules - Registering the
sqlfluff-lint
hook in.pre-commit-config.yaml
- Updating project dependencies to include SQLFluff
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
.sqlfluff | Configures SQLFluff for Postgres and style rules |
.pre-commit-config.yaml | Adds sqlfluff-lint hook under repos: |
Comments suppressed due to low confidence (2)
.pre-commit-config.yaml:127
- Ensure the indentation of this
- repo:
entry matches the otherrepos:
entries in the file to prevent YAML parsing errors.
- repo: https://github.com/sqlfluff/sqlfluff
.pre-commit-config.yaml:127
- Consider adding a CI step in your workflow to run
pre-commit run --all-files
so that SQLFluff linting is enforced on every pull request.
- repo: https://github.com/sqlfluff/sqlfluff
WHERE ratings.created <= (:'upto'::date + '1 day'::interval) | ||
) TO stdout WITH (format csv, delimiter E'\t') | ||
SELECT | ||
ratings.rating, |
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 will change the ordering of the columns, we don't want that.
- repo: https://github.com/sqlfluff/sqlfluff | ||
rev: 3.4.2 # or latest stable version | ||
hooks: | ||
- id: sqlfluff-fix | ||
name: SQLFluff Auto-Fix | ||
types: [sql] | ||
|
||
- repo: https://github.com/sqlfluff/sqlfluff | ||
rev: 3.4.2 | ||
hooks: | ||
- id: sqlfluff-lint | ||
name: SQLFluff Linter | ||
entry: sqlfluff lint | ||
language: python | ||
types: [sql] |
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.
Why is this here twice?
|
||
create index thing_latest_revision_idx ON thing(latest_revision); | ||
create index thing_latest_revision_idx on thing (latest_revision); |
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.
Why is this file requiring lower case, when the others require uppercase?
[sqlfluff] | ||
dialect = postgres | ||
exclude_rules = L031 | ||
templater = jinja |
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.
Do we need this? We don't use jinja
templater = jinja |
@@ -27,5 +27,6 @@ qrcode==7.4.2 | |||
requests==2.32.2 | |||
sentry-sdk==2.19.2 | |||
simplejson==3.19.1 | |||
sqlfluff==3.4.2 |
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 should go into requirements_test.py
Closes #10982
🧩 Problem
New contributors were blocked from initializing the repo due to a subtle syntax error in a .sql file (see #10979). These issues slipped by because the Postgres container caches its state and existing contributors didn’t experience the broken flow.
💡 Solution
Introduce sqlfluff as a SQL linter to catch future issues. Integrate it with our existing pre-commit system and configure it for Postgres.
🔧 Changes Included
Testing
Stakeholders