Skip to content

chore: add SQLFluff to pre-commit hooks#1774

Merged
CommanderStorm merged 9 commits intomaplibre:mainfrom
IvanPiatnishin:setup-sqlfluff-formatting
Apr 13, 2025
Merged

chore: add SQLFluff to pre-commit hooks#1774
CommanderStorm merged 9 commits intomaplibre:mainfrom
IvanPiatnishin:setup-sqlfluff-formatting

Conversation

@IvanPiatnishin
Copy link
Contributor

This adds SQLFluff with the PostgreSQL dialect to enforce SQL formatting rules via pre-commit.

@CommanderStorm CommanderStorm changed the title ✅ Pre-commit setup finalized with SQLFluff and hooks chore: add SQLFluff to pre-commit hooks Apr 7, 2025
@CommanderStorm
Copy link
Member

CommanderStorm commented Apr 7, 2025

the added CI-step is failing though. This needs to be fixed for mergability ^^
https://results.pre-commit.ci/run/github/105363726/1744040351.aYINARI8SCavxUA6wdemiQ

@sharkAndshark sharkAndshark requested a review from Copilot April 7, 2025 17:16

This comment was marked as resolved.

@sharkAndshark
Copy link
Collaborator

@IvanPiatnishin You may want to run commands to fix the formatting errors.

sqlfluff fix --dialect postgres demo/db/initdb/01.sql
sqlfluff fix --dialect postgres demo/db/initdb/03.sql
sqlfluff fix --dialect postgres martin/src/pg/scripts/query_available_function.sql

@CommanderStorm
Copy link
Member

CommanderStorm commented Apr 10, 2025

@sharkAndshark
Copy link
Collaborator

Or just run this to fix all .sql files

sqlfluff fix --dialect postgres

@nyurik
Copy link
Member

nyurik commented Apr 10, 2025

Note that ideally this should not just verify the style, but just like cargo fmt it should push changes to the pr

@nyurik
Copy link
Member

nyurik commented Apr 10, 2025

Note that this PR will not auto-run workflows because @IvanPiatnishin is a new github contributor. A workaround for this may be to create a PR in their own fork -- https://github.com/IvanPiatnishin/martin/compare/IvanPiatnishin:main...IvanPiatnishin:setup-sqlfluff-formatting?expand=1

@nyurik
Copy link
Member

nyurik commented Apr 10, 2025

On a more substantive notes - sqlfluff currently catches these two lints:

== [tests/fixtures/functions/function_special_characters.sql] FAIL
L:   1 | P:  32 | RF05 | Do not use special characters in identifiers.
                       | [references.special_chars]
== [martin/src/pg/scripts/query_available_tables.sql] FAIL
L:  11 | P:  39 | RF04 | Keywords should not be used as identifiers.

Both are actually OK for us because that's the whole idea of those tests - to see if our code will work correctly when the user does something weird. So we should disable these specific lints in the config somewhere

CommanderStorm added a commit that referenced this pull request Apr 11, 2025
Fixes the clippy lint issue from #1774
@CommanderStorm
Copy link
Member

I changed a few things about the setup in be89425

  • enabled fixing
  • re-added an removed comment
  • changed which rules we ignore (see below for rationale)

I think RF02 (Unqualified reference), RF04 (Keywords should not be used as identifiers) and RF05 (special characters in identifiers) they are interesting to keep as indicating possible correctness issues.
Not strong indicators, but a smell.

I would like them to be ignored instead:
https://docs.sqlfluff.com/en/3.2.3/configuration/ignoring_configuration.html

LT05 (line lenght) is mostly triggering on comments. => lets ignore this one

Copy link
Member

@nyurik nyurik left a comment

Choose a reason for hiding this comment

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

looks good! It seems we might need to remove PG 11 support as it is no longer installing? (it should probably be a separate PR to keep things separate)

@nyurik
Copy link
Member

nyurik commented Apr 13, 2025

Is it possible to ignore certain rules just for a single file, or maybe even for a specific line in a file? We could use something like that for keywords and special character tests.

@sharkAndshark
Copy link
Collaborator

sharkAndshark commented Apr 13, 2025

Seems sqlfluff support In-File Configuration Directives, we can do that

to ignore certain rules just for a single file, or maybe even for a specific line in a file

And certain line could be configured by this

-- Ignore all errors
SeLeCt  1 from tBl ;    -- noqa

-- Ignore rule CP02 & rule CP03
SeLeCt  1 from tBl ;    -- noqa: CP02,CP03

-- Ignore all parsing errors
SeLeCt from tBl ;       -- noqa: PRS

@CommanderStorm
Copy link
Member

CommanderStorm commented Apr 13, 2025

Is it possible to ignore certain rules just for a single file

Yes, see for example this file. We already do so:
https://github.com/maplibre/martin/blob/be894254fb564a4b6c3fa5e13ad3c5900588db2f/tests/fixtures/functions/function_special_characters.sql

Copy link
Member

@CommanderStorm CommanderStorm left a comment

Choose a reason for hiding this comment

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

Forgot to ACK this PR

@CommanderStorm CommanderStorm merged commit 2bdfd81 into maplibre:main Apr 13, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants