Skip to content

Add SQL style linter (Postgres-Extensions/linter) - #41

Merged
jnasbyupgrade merged 2 commits into
masterfrom
add-sql-linter
Aug 4, 2026
Merged

Add SQL style linter (Postgres-Extensions/linter)#41
jnasbyupgrade merged 2 commits into
masterfrom
add-sql-linter

Conversation

@jnasbyupgrade

@jnasbyupgrade jnasbyupgrade commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Vendor the shared org-wide SQL linter as a .vendor/linter submodule with a thin self-initializing lint.mk wrapper, wire it into the root Makefile scoped to sql/count_nulls.sql (the hand-written source; generated version snapshot files like sql/count_nulls--*.sql are excluded, per the linter's own DESIGN.md guidance on scoping LINT_TARGETS) plus test/, and add a make lint CI job that self-inits the submodule (checkout step deliberately does not pre-init it, so CI actually exercises that path).

sql/count_nulls.sql itself is clean. test/core/functions.sql had 11 pre-existing comment-line-prefix findings from two commented-out blocks (the boilerplate test__ function stub and a disabled array-type test). Rather than leave the lint job continue-on-error: true (which would mask any future real violation too, not just these known ones), those two blocks are now marked with the linter's own EXCLUDED CODE suppression convention (an alias for sql-lint:disable-block all, meant for exactly this case). make lint reports 0 findings and exits 0 for real, so the CI job runs with no continue-on-error and its pass/fail is a true signal.

This is a standalone, one-off change targeting master directly (not part of the in-progress update/upgrade-test PR stack).

Vendor the shared org-wide SQL linter as a .vendor/linter submodule with a
thin self-initializing lint.mk wrapper, wire it into the root Makefile
scoped to sql/count_nulls.sql (the hand-written source; generated version
snapshot files are excluded) and test/, and add a make lint CI job.

test/core/functions.sql currently has 11 pre-existing comment-line-prefix
findings (commented-out template/test code); the CI step is
continue-on-error for now so this integration doesn't block on cleaning up
that unrelated pre-existing debt.

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: a07e6f54-126e-4058-968c-7bd4d75570ce

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.

continue-on-error on the make lint step masked real findings permanently,
not just the known pre-existing ones -- any future violation would also
report green. The linter (Postgres-Extensions/linter) has a purpose-built
suppression convention for exactly this case: an `/* EXCLUDED CODE */`
marker on the opening line of a block comment, which is an alias for
`sql-lint:disable-block all` and skips comment-style checks for everything
inside that block.

Apply it narrowly to the two commented-out blocks in test/core/functions.sql
that triggered the 11 comment-line-prefix findings (the unused test__
boilerplate template, and the disabled array-type test). make lint now
finds 0 issues and exits 0 for real, so continue-on-error can be dropped
entirely -- the lint job's pass/fail is now a true signal again.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown

Claude finished @jnasbyupgrade's task in 3m 5s —— View job


Code Review: Add SQL style linter (Postgres-Extensions/linter)

  • Gather context (PR diff, changed files, CLAUDE.md)
  • Review Makefile / lint.mk wiring
  • Review .gitmodules / submodule setup
  • Review CI workflow change
  • Review test/core/functions.sql change
  • Post final review

Verification done: confirmed .vendor/linter is a proper gitlink (pinned at b40aaf7), and pulled the actual 🧹 SQL lint job log from run 30949965872 — it shows make lint self-initializing the submodule from a plain (non-submodule) checkout via git submodule update --init -- .vendor/linter, then running sql-lint sql/count_nulls.sql test/ cleanly. That's exactly what the PR claims, and it's a good instinct to prove the self-init path actually works rather than trusting it.

Overall this is a clean, well-scoped, minimal-footprint integration. A few notes:

1. PR description is stale re: continue-on-error
The PR body says the lint job is continue-on-error: true "for now," but .github/workflows/ci.yml as merged has no such setting — the job runs as a hard gate. This lines up with commit 56f6c03 ("Fix lint job to be an honest signal") replacing that escape hatch with the EXCLUDED CODE marker fix instead. Not a code problem, just worth updating the PR description so reviewers/future readers aren't misled about there being a soft-fail safety net that isn't actually there.

2. Submodule pinned to an arbitrary commit, not a tag
.vendor/linter is pinned at commit b40aaf7 rather than a released tag. Minor — if the linter repo publishes version tags, pinning to one would make future "what version are we on" and upgrade diffs easier to reason about. Not blocking.

3. Scoping and markers look correct

  • LINT_TARGETS = sql/count_nulls.sql test/ in the Makefile correctly excludes the generated sql/count_nulls--*.sql snapshot files, with a clear comment explaining why.
  • The two EXCLUDED CODE markers added in test/core/functions.sql (lines 15 and 183) are minimal, targeted, and each carries a short rationale for why the commented-out code exists (unused boilerplate stub; doesn't work for array types) — good practice, since a bare EXCLUDED CODE marker with no explanation would just be trading one mystery for another.
  • No other file under test/ needed similar treatment (confirmed via the CI log — the lint run covered all of test/ and passed clean).

No functional bugs found. This is ready to merge as-is; the two notes above are polish, not blockers.

@jnasbyupgrade
jnasbyupgrade merged commit 1364af3 into master Aug 4, 2026
19 checks passed
@jnasbyupgrade
jnasbyupgrade deleted the add-sql-linter branch August 4, 2026 21:08
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