Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SQL Linter using Squawk #94

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

febrianyuwono
Copy link
Contributor

@febrianyuwono febrianyuwono commented Nov 14, 2023

Adding an integration to Squawk (website | repo) so we can double check if there are any database migrations that would cause unexpected downtime.

README.md Outdated Show resolved Hide resolved
src/SquawkLinter.php Outdated Show resolved Hide resolved
->setCode($rulename)
->setPath(idx($item, 'source', $path))
->setLine($line)
->setChar(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have expecting $column to be used here to set the "character" offset, but it looks like it's being used to adjust the $line above?

Copy link
Contributor Author

@febrianyuwono febrianyuwono Nov 15, 2023

Choose a reason for hiding this comment

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

the $column here is showing how many rows are being warned by the linter
for example you have drop column like this:

   1 ALTER TABLE
   2   table_name
   3 DROP COLUMN
   4   column_name;
   5 
   6 -- test some column with spaces
   7               ALTER TABLE table_name DROP COLUMN column_name;
   8 
   9 
  10 -- test column with comment
  11 /* bla3 */ ALTER TABLE table_name DROP column column_name;

the first one will have line 1 column 0
the second one will have line 5 column 2 (so I added line + column - 1)
the third one will have line 8 column 3 (8+3-1 = 10)

so the calculated line is used to show which line is the starting one (includes comment)

and the char offset is not shown in the linter, so it's put as one

Copy link
Collaborator

@jparise jparise left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks for the contribution!

@jparise jparise merged commit 57a3165 into pinterest:master Nov 16, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants