Add SQL style linter (vendored Postgres-Extensions/linter) - #16
Open
jnasbyupgrade wants to merge 1 commit into
Open
Add SQL style linter (vendored Postgres-Extensions/linter)#16jnasbyupgrade wants to merge 1 commit into
jnasbyupgrade wants to merge 1 commit into
Conversation
Vendor Postgres-Extensions/linter as a git submodule at .vendor/linter,
following the same pattern already adopted in cat_tools: a thin
self-initializing lint.mk hand-off (so `make lint` works right after a
plain `git clone`, no --recurse-submodules needed), LINT_TARGETS scoped
to sql/object_reference.sql and test/ (excluding the frozen, never
hand-edited versioned install files under sql/, e.g.
object_reference--0.1.0.sql/--stable.sql), and a CI job that runs
`make lint` directly -- the same entry point a developer uses locally
-- so the self-init logic is actually exercised, not just the rule
checking.
The `include lint.mk` is guarded on .git being present: a tarball build
(PGXN distribution, `git archive` with no .git) has no submodule to
initialize, and Make resolves every `include` before running any
target regardless of which one was requested, so an unguarded rule
would break `make`/`make install` entirely for a tarball build, not
just `make lint`.
Fixes the real pre-existing style findings this first run turned up
(52 total): most were commented-out SQL marked as prose comments
instead of using the linter's `EXCLUDED CODE` disabled-code convention
(missing " * " prefixes flagged as comment-line-prefix/comment-opening
violations); one COPY data block's `secondary` column intentionally
mirrors pg_catalog's own type display name ("integer" for int4) rather
than following prefer-short-type, so it's suppressed via a scoped
disable-block region instead of being "fixed" into incorrect test data.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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:
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 |
This was referenced Aug 5, 2026
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
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.
Summary
Postgres-Extensions/linteras a git submodule at.vendor/linter, with a thin self-initializinglint.mkhand-off (somake lintworks right after a plaingit clone, no--recurse-submodulesneeded) — same pattern already adopted incat_tools.LINT_TARGETSis scoped tosql/object_reference.sqlandtest/, excluding the frozen, never-hand-edited versioned install files undersql/(object_reference--0.1.0.sql,object_reference--stable.sql) — those are auto-generated / release-frozen per this repo's own convention, so linting them would produce permanent, unfixable findings..github/workflows/ci.ymlwith alintjob that runsmake lintdirectly (the exact command a developer runs locally), checking out without submodules so the self-init logic inlint.mkis actually what's exercised — not papered over with asubmodules: truecheckout. (ci.ymldid not exist yet onmaster; this PR adds it with just this one job, deliberately minimal/scoped to the linter. It doesn't touch the fuller CI restructure happening in a separate, parallel PR.)include lint.mkbehindifneq ($(wildcard .git),): a tarball build (PGXN distribution, or anygit archivecheckout with no.git) has no submodule to initialize, and Make resolves everyincludebefore running any target regardless of which one was requested — so an unguarded self-init rule would breakmake/make installentirely for a tarball build, not justmake lint. Verified: built a realgit archive HEAD | tar -xcheckout with no.gitand confirmed plainmakeandmake installboth still succeed; also confirmed (by temporarily removing the guard) that without it the same tarball build fails hard.Pre-existing findings
The very first
make lintrun against the real currentsql/object_reference.sql/test/content found 52 real findings (not a suspiciously clean first pass). All were fixed in this PR rather than suppressed wholesale:/*) rather than using the linter'sEXCLUDED CODEdisabled-code convention — an alias forsql-lint:disable-block allmeant exactly for this case. Converted each to/* EXCLUDED CODE[: reason](preserving existingTODO:context as the reason where present).test/sql/event_trigger.sql) was just missing a space after*— fixed directly.test/helpers/object_table.sql, aCOPY ... FROM STDINdata block) was a false positive: thesecondarycolumn's valueintegeris literallypg_catalog's own display name forint4(format_type), which the test data intentionally preserves as-is — not a code style choice, and not something that could be "fixed" without corrupting the test data or the liveCOPYpayload. Since inline--suppression isn't usable insideCOPYdata (it would become part of the literal row), this is scoped-suppressed via the linter's region-suppression mechanism (-- sql-lint:disable-block prefer-short-type: .../-- sql-lint:enable-block) wrapping theCOPYblock, rather thancontinue-on-erroron the CI job.CI wiring verification
Deliberately introduced one obvious style violation, pushed, and confirmed the
lintjob went red with the expected finding (test/deps.sql:11: [comment-line-prefix] ...); then reverted it and confirmed green again. Branch history was then cleaned up (the temporary violation + revert collapsed out) since it only mattered as a live CI proof, not as PR history — the linked CI run above verifies the same thing.Test plan
make lint— 0 findings against currentsql/object_reference.sql/test/make install/make installcheck— confirmed comment-only edits don't change behavior (same pre-existing, unrelated 7 test failures occur identically with and without this PR's changes — acat_tools/count_nullsversion-vs-expected-output mismatch in this environment, not caused by this PR)git archive HEAD | tar -xinto a directory with no.git, confirmed plainmakeandmake installsucceed; confirmed (by removing the guard) that they'd otherwise faillintjob goes red on an injected violation, green once removed🤖 Generated with Claude Code