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

Support Auto-Fix with Sqlfluff #2784

Open
BrunoJuchli opened this issue Jul 11, 2023 · 11 comments
Open

Support Auto-Fix with Sqlfluff #2784

BrunoJuchli opened this issue Jul 11, 2023 · 11 comments
Labels
enhancement New feature or request nostale This issue or pull request is not stale, keep it open

Comments

@BrunoJuchli
Copy link

BrunoJuchli commented Jul 11, 2023

Problem

Currently megalinter Sqlfluff integration does not support fixing of files.

I have tried different combinations, like:

passing fix as argument

SQL_SQLFLUFF_ARGUMENTS: "fix"
SQL_SQLFLUFF_CLI_LINT_MODE: "file"

=> Error says fix is not a valid file path

reconfiguring executable

SQL_SQLFLUFF_CLI_EXECUTABLE: ['sqlfluff', 'fix']
SQL_SQLFLUFF_CLI_LINT_MODE: "file"

=> Error says 'lint' is not a valid file path

User Error: Specified path does not exist. Check it/they exist(s): lint.

Proposed Solution

Use the standard way of applying auto-fixes, by either:

-adding 'SQL_SQLFLUFF ' to 'APPLY_FIXES'
-setting 'APPLY_FIXES' to 'all'

When auto-fixing is enabled, the following should happen:

  • instead of passing 'lint', passes 'fix' to sqlfluff executable.
  • switches 'SQL_SQLFLUFF_CLI_LINT_MODE' to file (sqlfluff fix only supports a single file or folder path as argument)

Alternative Configurability

Add a new configurable variable

SQL_SQLFLUFF_FIX with a default value of 'false'.

@BrunoJuchli BrunoJuchli added the enhancement New feature or request label Jul 11, 2023
@nvuillam
Copy link
Member

nvuillam commented Jul 11, 2023

@BrunoJuchli thanks for the analysis and propositions :)

When sqlfluff is called with sqlfluff fix, does it also lint ?

There are ways to replace lint by fix just in descripto

image

but replacing the cli_lint_mode could be done only in a Python class associates to the linter descriptor, something like in overridden method before_lint_files

if self.apply_fixes is True:
   self.cli_lint_mode = file

image

Would you like to make a PR ? :)

@BrunoJuchli
Copy link
Author

@nvuillam
Thanks for the input. Sorry it took me so long to respond. Initially I was under the impression that I could also somehow remove the lint argument without actually modifying the mega-linter code. So I waited until I had some time on my hands to try it out... but of course now I know better ^^

re whether sqlfluff fix also lints: I interpret your question to mean "does it list errors it cannot fix".
I had to do a search on this, as it isn't obvious to me either :D
So finally I found the answer on their slack channel. For fix + lint we should call it as follows:

sqlfluff fix --show-lint-violations

However, not sure if it actually works. At least for a specific scenario it seems to fail to report linting errors: sqlfluff/sqlfluff#4871

Re providing a PR: last time I worked with python was more than 10 years ago... so I fear it might take me quite long => I don't want my boss to get out his Kloppe ;-)

image

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Aug 17, 2023
@nvuillam
Copy link
Member

@BrunoJuchli I learnt python to build MegaLinter, the code is simple :p

And i'll help if necessary :)

@github-actions github-actions bot removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Aug 22, 2023
@P107RP
Copy link

P107RP commented Aug 25, 2023

@nvuillam I am unsure if it is related to the reported problem, but sqlfluff fix is not working for mega-linter-runner either.
I tested the following:
mega-linter-runner -r v7 --fix
mega-linter-runner -r v7 --flavor cupcake --fix
and
mega-linter-runner -r v7 -e 'APPLY_FIXES=all'
Errors are reported correctly however using --fix parameters is not fixing any of SQL errors.
If I run locally sqlfluff fix some are fixed.
Have you noticed similar issues?

@nvuillam
Copy link
Member

@P107RP today sqlfluff fix is not called, it would require a PR ^^

@P107RP
Copy link

P107RP commented Aug 28, 2023

@nvuillam I think it is a very handy feature ;)
Do you have any plans to implement this, with a next version maybe?

@nvuillam
Copy link
Member

I have a crazy backlog, not only on MegaLinter, so i'll probably do it someday but that might be not soon ^^

If someone submits a PR I'll be glad to accept it :)

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Sep 28, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 13, 2023
@nvuillam nvuillam reopened this Oct 14, 2023
@nvuillam nvuillam added nostale This issue or pull request is not stale, keep it open and removed O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity labels Oct 14, 2023
@YElyousfi
Copy link

The issue went stale but any chance this will get implemented soon?

@nvuillam
Copy link
Member

nvuillam commented Jan 29, 2024

It depends on if a generous contributor wants to spent some time on it to submit a PR :)
On my side... I'm drowning sorry ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request nostale This issue or pull request is not stale, keep it open
Projects
None yet
Development

No branches or pull requests

4 participants