linter: add file-level override for assume_in_transaction#996
linter: add file-level override for assume_in_transaction#996reteps wants to merge 1 commit intosbdchd:masterfrom
assume_in_transaction#996Conversation
Add `-- squawk-disable-assume-in-transaction` and `-- squawk-enable-assume-in-transaction` comments to override the global `assume_in_transaction` setting on a per-file basis. This is useful when a migration tool wraps files in transactions by default but specific files need to opt out (e.g., for `CREATE INDEX CONCURRENTLY`). Closes sbdchd#990 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
👷 Deploy request for squawkhq pending review.Visit the deploys page to approve it
|
reteps
changed the title
assume_in_transaction
| #[test] | ||
| fn squawk_disable_assume_in_transaction_allows_begin_commit() { | ||
| let sql = r#" | ||
| -- squawk-disable-assume-in-transaction |
There was a problem hiding this comment.
I wonder if a more general config style comment would be better since if we want to add another setting, we don't have to sprinkle in another comment
There was a problem hiding this comment.
squawk-disable/enable [space] assume-in-transaction?
There was a problem hiding this comment.
Thinking about it some more, I feel like as a setting assume-in-transaction isn't great
It's essentially a hack that avoids having to wrap the migration in a begin and commit
I think adding/removing begin/commit would be better than using assume-in-transaction more
There was a problem hiding this comment.
can you say more? I'm happy to try and implement anything that you think has some merit here.
So by default, the whole file is one "transaction" if assume is on. Otherwise, each line is separate.
Are you suggesting these as new directives, and if defined in the file, they take precedence over the previous behavior and let you split up a file into multiple transactions using begin/end markers?
There was a problem hiding this comment.
Is your migration system open source? Wondering how the flow works
It seems like we could add this enable/disable via the assume-in-transaction comment
OR
the system could wrap or not wrap migrations in begin/commit pairs
There was a problem hiding this comment.
Yes - here is our system: https://github.com/PrairieLearn/PrairieLearn/blob/master/packages/migrations/README.md
.sql migrations are run inside a transaction by default. If your migration cannot run inside a transaction (for instance, if it uses CREATE INDEX CONCURRENTLY), you can add a special annotation comment to the file:
-- prairielearn:migrations NO TRANSACTION
CREATE INDEX CONCURRENTLY ...;
There was a problem hiding this comment.
Ah gotcha, I think it makes sense as a feature.
I think before we merge, could you update it so it only has the disable comment, I don't think we need the enable side of things (at least not yet?)
There was a problem hiding this comment.
Do you prefer option (A)
-- squawk-disable assume-in-transaction
or option (B)
-- squawk-disable-assume-in-transaction
as the syntax?
There was a problem hiding this comment.
Yeah I think -- squawk-disable-assume-in-transaction is good!
|
Marking as draft till this is revamped given @sbdchd's preferences on directions to go in. |
| const ENABLE_ASSUME_IN_TRANSACTION: &str = "squawk-enable-assume-in-transaction"; | ||
|
|
||
| pub fn find_transaction_override(file: &SyntaxNode) -> Option<bool> { | ||
| for event in file.preorder_with_tokens() { |
There was a problem hiding this comment.
also seems like we'd only want to check the first 20 lines rather than have to look at every comment in the file (including ones that are nested inside other nodes)
There was a problem hiding this comment.
ignore is a little different since lint errors can occur ~anywhere
2 participants
Summary
-- squawk-disable-assume-in-transactionand-- squawk-enable-assume-in-transactionSQL comments to override the globalassume_in_transactionsetting on a per-file basissquawk-ignore/squawk-ignore-filecomment conventions (supports both--and/* */styles, plus trailing comments)CREATE INDEX CONCURRENTLY)Closes #990
Testing
find_transaction_override)squawk-disable-assume-in-transactionallowsCREATE INDEX CONCURRENTLYwhenassume_in_transactionis globally enabledsquawk-disable-assume-in-transactionallows explicitBEGIN/COMMITwhenassume_in_transactionis globally enabledsquawk-enable-assume-in-transactionflagsBEGIN/COMMITwhenassume_in_transactionis globally disabled🤖 Generated with Claude Code
Of course, disclaimer, code was written with Claude 4.6 Opus. Feel free to use this PR as a starting point, request changes, or close this out. Thank you!
This is useful to us for the exact reasons I spelled out in #990 -- this will let us turn on more config flags. I changed the comment messages themselves because i felt this better lined up with how the comments worked. I am of course open to other names / ways of configuring on/off.