Add sql-lint: style linter for PostgreSQL SQL files#49
Open
jnasbyupgrade wants to merge 6 commits into
Open
Conversation
Adds a Perl-based style linter enforcing this project's SQL coding
conventions (block comment formatting, leading-comma style, short type-name
preferences). Adds `make lint`, gated in CI (`lint` job) alongside the
existing test matrix.
The linter supports both plain .sql files and cat_tools' .sql.in template
convention (the directory walk in bin/sql-lint matches both extensions), and
its generated-file skip check matches a first line containing both
"GENERATED" and "DO NOT EDIT" in either order, so it recognizes this
project's build marker ("GENERATED FILE! DO NOT EDIT!"). See lint/sql/
DESIGN.md's "Portability" section.
lint.mk gained a LINT_TARGETS override (default sql/ test/, unchanged). The
root Makefile sets it to sql/cat_tools.sql.in, sql/omit_column.sql, and
test/ -- excluding version-specific install/update scripts, which are frozen
once released (never hand-edited again per this repo's SQL file conventions)
and would otherwise produce permanent, unfixable findings.
Also fixes the resulting findings in the current source so `make lint`
passes clean: two inline /* */ comments converted to --, two stacked -- runs
converted to /* */ blocks, a borrowed SELECT list (pg_all_foreign_keys)
converted from trailing to leading commas, and a commented-out test fixture
marked with the EXCLUDED CODE convention instead of being reformatted line by
line. sql/cat_tools--0.3.0.sql.in is regenerated to match (make install &&
make test both verified green after these changes).
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- lint/sql/test/fixtures/prefer-short-type.{bad,good}.sql: rename the id
column to table_name_id.
- comment-stacked-dashes: allow up to 2 consecutive -- lines; only 3+ must
become a /* */ block. Updated fixtures/docs and reverted the two
cat_tools.sql.in spots that were converted to /* */ under the old
threshold back to -- (both are exactly 2 lines).
- Added a new region-suppression pair, disable-block/enable-block on their
own -- lines, for silencing every rule across a chunk of real code (as
opposed to the existing /* */-anchored disable-block, which only applies
inside a comment and auto-closes at the comment's own */). Used it to wrap
the borrowed pg_all_foreign_keys view in cat_tools.sql.in, which is also
reverted back to its original trailing-comma form -- that borrow was
intentionally left unreformatted.
- Dropped the sed-generation-artifact regex comment in cat_tools.sql.in
(object_type case expression); it was a personal note on how the WHEN
clauses were originally generated, not documentation worth keeping.
- test/sql/pg_depends.sql: removed the commented-out func_calls view instead
of marking it EXCLUDED CODE.
- sql.mk: version-specific install/update scripts (sql/cat_tools--X.Y.Z.sql)
now get an extra "VERSIONED FILE!" tag ahead of the existing GENERATED
FILE marker, so it's obvious at a glance that a file is a frozen snapshot
that must never be hand-edited -- distinct from cat_tools.sql itself. The
substitution is now anchored to whole-line @generated@ markers rather than
a bare substring match, which incidentally also stops it from clobbering
the handful of @generated@ occurrences that are trailing annotations on
real code lines in cat_tools.sql.in (e.g. on the confdeltype CASE
expression) rather than standalone section-divider markers.
make -C lint test, make lint, make install, and make test all verified
green after these changes.
Replace the fully-copied lint/ directory with a git submodule at .vendor/linter, pointing at the new https://github.com/Postgres-Extensions/ linter -- the actual linter code, its tests, and its docs now live in exactly one place, shared across every Postgres-Extensions project, instead of a separate copy per consuming repo. The entire local footprint for consuming it is one small lint.mk: it self-initializes the submodule (git submodule update --init) if needed, then hands off to the vendored repo's own lint.mk. This means `make lint` works right after a plain `git clone` with no --recurse-submodules step, and the Makefile/CI don't need to know anything about the linter beyond that one hand-off -- everything else (rules, tests, README, LICENSE) lives in the vendored repo. CI's lint job now checks out the repo WITHOUT submodules and just runs `make lint` -- the same command a developer would run locally -- which is what actually proves the self-init in lint.mk works, rather than papering over it with a submodules: true checkout. The linter's own test suite (fixtures + scanner edge cases) moved to being that repo's own CI's responsibility, so the lint job here dropped its separate `make -C lint test` step. Verified make lint (after deinit-ing the submodule to simulate a fresh clone), make install, and make verify-results all green.
Bumps .vendor/linter to Postgres-Extensions/linter@b8632c2, which also fixes a bug the missing comment surfaced: annotate_region_suppressions() captured the rule id with (\S+), so a same-line reason after a colon (no space before it, e.g. `all: some reason`) got swallowed into the captured token -- "all:" never equals "all", so the region silently stopped suppressing anything. Fixed upstream to anchor the capture to the rule id itself. make lint, make install, and make verify-results all still green.
… rule Simpler than the previous approach: instead of the shared sql/%.sql build rule branching on $* (whether the stem contains --), the copy step that makes sql/cat_tools--<version>.sql.in from sql/cat_tools.sql.in now tags its own @generated@ divider markers as 'VERSIONED FILE! @generated@' (anchored to the whole line, so the copy doesn't touch the handful of inline coincidental @generated@ occurrences on real code lines either). The shared build rule just resolves whichever form -- tagged or bare -- it finds, with no knowledge of which file it's building. One consequence of tagging in the copy step rather than the build rule: the echo-inserted top/bottom boundary markers (added fresh at build time, not part of the .sql.in content) stay plain even for a version-file build -- only the ~53 internal divider markers that came from the copied content carry the tag. Still plenty visible at a glance, just not on literally every marker in the file. Regenerated sql/cat_tools--0.3.0.sql.in to match. make lint, make install, and make verify-results all still green.
@generated@ itself is what the shared build rule's sed substitutes into the '-- GENERATED FILE! DO NOT EDIT!' marker -- prefixing 'VERSIONED FILE! ' before it meant the shared rule had to special-case matching that whole prefixed string separately from a bare @generated@ line, duplicating the substitution. Appending ' VERSIONED FILE!' after @generated@ instead means the shared rule's substitution -- now anchored to just the START of the line, not the whole line -- resolves the @generated@ prefix exactly the same way whether or not a version tag follows, and whatever trailing text is present just rides along into the final marker text. So the two build recipes (the sql/%.sql pattern rule and the EXTENSION_VERSION_FILES override) both drop back to the single substitution they had before any of the VERSIONED FILE! work -- just with the anchor now caring about line start rather than nothing at all, which is what actually avoids clobbering the handful of inline @generated@ occurrences that are trailing annotations on real code lines. Regenerated sql/cat_tools--0.3.0.sql.in to match. make lint, make install, and make verify-results all still green.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a Perl-based style linter enforcing this project's SQL coding conventions (block comment formatting, leading-comma style, short type-name preferences). Adds `make lint`, gated in CI (`lint` job) alongside the existing test matrix.
The linter supports both plain .sql files and cat_tools' .sql.in template convention (the directory walk in bin/sql-lint matches both extensions), and its generated-file skip check matches a first line containing both "GENERATED" and "DO NOT EDIT" in either order, so it recognizes this project's build marker ("GENERATED FILE! DO NOT EDIT!"). See lint/sql/DESIGN.md's "Portability" section for more on these choices.
lint.mk gained a LINT_TARGETS override (default sql/ test/, unchanged). The root Makefile sets it to sql/cat_tools.sql.in, sql/omit_column.sql, and test/ -- excluding version-specific install/update scripts, which are frozen once released (never hand-edited again per this repo's SQL file conventions) and would otherwise produce permanent, unfixable findings.
Also fixes the resulting findings in the current source so `make lint` passes clean: two inline /* / comments converted to --, two stacked -- runs converted to / */ blocks, a borrowed SELECT list (pg_all_foreign_keys) converted from trailing to leading commas, and a commented-out test fixture marked with the EXCLUDED CODE convention instead of being reformatted line by line. sql/cat_tools--0.3.0.sql.in is regenerated to match. `make install` and `make test` both verified green locally after these changes.