Skip to content

linter: add require-table-schema rule#1046

Merged
kodiakhq[bot] merged 3 commits intosbdchd:masterfrom
Flaiers:patch-require-table-schema
Apr 15, 2026
Merged

linter: add require-table-schema rule#1046
kodiakhq[bot] merged 3 commits intosbdchd:masterfrom
Flaiers:patch-require-table-schema

Conversation

@Flaiers
Copy link
Copy Markdown
Contributor

@Flaiers Flaiers commented Apr 7, 2026

Enforce that all table DDL (CREATE TABLE, CREATE TABLE AS, ALTER TABLE, DROP TABLE) uses schema-qualified names to prevent ambiguity from search_path changes.

@netlify
Copy link
Copy Markdown

netlify bot commented Apr 7, 2026

👷 Deploy request for squawkhq pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit edf19e3

Comment on lines 469 to 474
pub fn with_all_rules() -> Self {
let rules = all::<Rule>().collect::<FxHashSet<_>>();
let rules = all::<Rule>()
.filter(|r| !r.is_opt_in())
.collect::<FxHashSet<_>>();
Linter::from(rules)
}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

How bad is this? I'm afraid that adding require-table-schema will affect many current squawk users (the linter will fail after the update).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yeah I think we'll want to have it disabled by default, I'm not 100% sure of people's usage patterns

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looking at where we call with_all_rules, it's only used in the LSP Server & Playground (via Wasm)

So I think we could skip this is_opt_in method

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What about without_rules? Just always insert RequireTableSchema into exclude_set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Yeah I think so, without_rules is disabling the rules that are explicitly passed in

@sbdchd sbdchd added the automerge automerge with kodiak label Apr 15, 2026
@sbdchd
Copy link
Copy Markdown
Owner

sbdchd commented Apr 15, 2026

Oh my mistake, we do need that is_opt_in method

Copy link
Copy Markdown
Owner

@sbdchd sbdchd left a comment

Choose a reason for hiding this comment

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

Thank you for setting this up!

@kodiakhq kodiakhq bot merged commit 0affaf6 into sbdchd:master Apr 15, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge automerge with kodiak

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants