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

[NOREF] Add SQLFluff linter #2904

Merged
merged 2 commits into from
Nov 26, 2024
Merged

[NOREF] Add SQLFluff linter #2904

merged 2 commits into from
Nov 26, 2024

Conversation

ClayBenson94
Copy link
Collaborator

@ClayBenson94 ClayBenson94 commented Nov 26, 2024

NOREF

Description

  • Adds new SQLFluff linter to the pre-commit config
  • Run linter

This will lint all .sql files that don't live in the /migrations folder (since we can't edit old migrations, running this linter on that folder would result in that)

Note

We're using the SQLFluff core rules by default, but feel free to look at all the rules to see if anything else would be useful!

How to test this change

  • Modify a .sql file (somewhere in /pkg would be easiest) to update something like indentation, casing, or format
  • Run pre-commit run --all sqlfluff-fix
  • Ensure that the file was fixed / modified

Important

There's a lot of files touched in this PR - exhaustively going over each one in detail probably isn't reasonable, but I recommend looking over each generally to make sure the types of changes are what we're comfortable with (spacing? formatting? indentation? capitalization?)

PR Author Checklist

  • I have provided a detailed description of the changes in this PR.
  • I have provided clear instructions on how to test the changes in this PR.
  • I have updated tests or written new tests as appropriate in this PR.

PR Reviewer Guidelines

  • It's best to pull the branch locally and test it, rather than just looking at the code online!
  • When approving a PR, provide a reason why you're approving it
    • e.g. "Approving because I tested it locally and all functionality works as expected"
    • e.g. "Approving because the change is simple and matches the Figma design"
  • Don't be afraid to leave comments or ask questions, especially if you don't understand why something was done! (This is often a great time to suggest code comments or documentation updates)
  • Check that all code is adequately covered by tests - if it isn't feel free to suggest the addition of tests.

@ClayBenson94 ClayBenson94 requested review from a team as code owners November 26, 2024 13:35
@ClayBenson94 ClayBenson94 requested review from samoddball and mynar7 and removed request for a team November 26, 2024 13:35
Copy link
Contributor

@mynar7 mynar7 left a comment

Choose a reason for hiding this comment

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

Nice.

UNION
SELECT modified_by AS username
FROM trb_request_system_intakes
) AS combined_usernames
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: good lord what is this? Unions pulling data from every table?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mynar7 , yeah this was for migrating to the user account table. We needed to get the usernames that might not have a user account representation.

@ClayBenson94
Copy link
Collaborator Author

For some reason this PR is now failing the shell-check step, even though that wasn't modified here 🤔

I might need to make another PR to address that

https://github.com/CMS-Enterprise/easi-app/actions/runs/12031770967/job/33545711634#step:8:169

- id: sqlfluff-fix
exclude: ^migrations/
- id: sqlfluff-lint
exclude: ^migrations/
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I've addressed ignoring old migrations if you want to lint any new ones
CMS-Enterprise/mint-app#1538.

Note, anytime we upgrade linter versions, we likely would want to update the regex to not change indentation etc

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm gonna merge this PR as-is and we can loop back on enabling this for migrations later, if that's alright!

Copy link
Contributor

Choose a reason for hiding this comment

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

that's great with me!

@ClayBenson94 ClayBenson94 merged commit 628fff1 into main Nov 26, 2024
12 checks passed
@ClayBenson94 ClayBenson94 deleted the NOREF/add_sqlfluff_linter branch November 26, 2024 16:50
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.

3 participants