Skip to content

Add SQL style linter (vendored Postgres-Extensions/linter) - #16

Open
jnasbyupgrade wants to merge 1 commit into
Postgres-Extensions:masterfrom
jnasbyupgrade:add-linter
Open

Add SQL style linter (vendored Postgres-Extensions/linter)#16
jnasbyupgrade wants to merge 1 commit into
Postgres-Extensions:masterfrom
jnasbyupgrade:add-linter

Conversation

@jnasbyupgrade

Copy link
Copy Markdown
Contributor

Summary

  • Vendors Postgres-Extensions/linter as a git submodule at .vendor/linter, with a thin self-initializing lint.mk hand-off (so make lint works right after a plain git clone, no --recurse-submodules needed) — same pattern already adopted in cat_tools.
  • LINT_TARGETS is scoped to sql/object_reference.sql and test/, excluding the frozen, never-hand-edited versioned install files under sql/ (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.
  • Adds .github/workflows/ci.yml with a lint job that runs make lint directly (the exact command a developer runs locally), checking out without submodules so the self-init logic in lint.mk is actually what's exercised — not papered over with a submodules: true checkout. (ci.yml did not exist yet on master; 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.)
  • Guards include lint.mk behind ifneq ($(wildcard .git),): a tarball build (PGXN distribution, or any git archive checkout 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 self-init rule would break make/make install entirely for a tarball build, not just make lint. Verified: built a real git archive HEAD | tar -x checkout with no .git and confirmed plain make and make install both 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 lint run against the real current sql/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:

  • Most were commented-out SQL that had been written as an ordinary block comment (needing " * " prefixes / no text after opening /*) rather than using the linter's EXCLUDED CODE disabled-code convention — an alias for sql-lint:disable-block all meant exactly for this case. Converted each to /* EXCLUDED CODE[: reason] (preserving existing TODO: context as the reason where present).
  • One genuine prose comment (test/sql/event_trigger.sql) was just missing a space after * — fixed directly.
  • One finding (test/helpers/object_table.sql, a COPY ... FROM STDIN data block) was a false positive: the secondary column's value integer is literally pg_catalog's own display name for int4 (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 live COPY payload. Since inline -- suppression isn't usable inside COPY data (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 the COPY block, rather than continue-on-error on the CI job.

CI wiring verification

Deliberately introduced one obvious style violation, pushed, and confirmed the lint job 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 current sql/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 — a cat_tools/count_nulls version-vs-expected-output mismatch in this environment, not caused by this PR)
  • Tarball build check: git archive HEAD | tar -x into a directory with no .git, confirmed plain make and make install succeed; confirmed (by removing the guard) that they'd otherwise fail
  • Live CI: lint job goes red on an injected violation, green once removed

🤖 Generated with Claude Code

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>
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 14bef208-b8c3-44c9-b174-c24fe9489e05

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

1 participant