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

fix: sqlfluff to allow config file other than .sqlfluff #4562

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

keatmin
Copy link

@keatmin keatmin commented Jul 25, 2023

Closes #4554

This PR allows config file other than .sqlfluff to be used as listed:

  • setup.cfg
  • tox.ini
  • pep8.ini
  • pyproject.toml

Adding a new variable ale_sql_sqlfluff_config_file to be added to allow sqlfluff to use config file nearest to the buffer.

Copy link
Contributor

@pbnj pbnj 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 to me.

@@ -19,7 +22,7 @@ function! ale_linters#sql#sqlfluff#Command(buffer) abort
\ ale#Escape(l:executable)
\ . ' lint'

let l:config_file = ale#path#FindNearestFile(a:buffer, '.sqlfluff')
let l:config_file = ale#path#FindNearestFile(a:buffer, ale#Var(a:buffer, 'sql_sqlfluff_config_file'))
Copy link
Member

Choose a reason for hiding this comment

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

Instead of manually specifying the location of the configuration file or adding an option for which file to use, I wonder if we can edit the linter command by prefixing the command with a work directory so sqlfluff automatically determines which configuration file to load. The project specifies some behaviour for searching for configuration files. https://docs.sqlfluff.com/en/2.1.4/configuration.html#nesting

Copy link
Author

@keatmin keatmin Aug 2, 2023

Choose a reason for hiding this comment

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

@w0rp tbh that would be much preferred.

My original idea was to replace %t with %s in the linter and fixer and let the tool determines which configuration to load. The reason why it didn't load was because the linter/fixer was pointed to %t and it couldn't find any config files. Alternatively using cwd could probably help ?

I just recently migrated to ale from null-ls and not the author of the original code, so I wasn't sure if replacing the command with %s for both linter and fixer will have unintended consequences so I tried to make the code to be as backward compatible as possible.

Appreciate the guidance 🙏🏽 !

Copy link
Member

Choose a reason for hiding this comment

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

You might be able to get it to work by adding 'cwd': '%s:h' to the linter config. You can look at the rstcheck.vim file for an example.

Choose a reason for hiding this comment

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

You might be able to get it to work by adding 'cwd': '%s:h' to the linter config. You can look at the rstcheck.vim file for an example.

I've tried adding this, but doesn't really help in my case; e.g., my directory structure would be something like

repo  # Project working directory
├── pyproject.toml  # The config I want applied
└── queries
    └── query.sql  # The file I'm editing

So, if I start vim from the project root (in this case, repo), open queries/query.sql, then cwd for the linter is set to repo/queires instead of repo, and sqlfluff fails to find pyproject.toml; i.e., the lint command is

['bash', '-c', 'cd ''/repo/queries'' && ''sqlfluff'' lint ...]

instead of the desired

['bash', '-c', 'cd ''/repo'' && ''sqlfluff'' lint ...]

If I run sqlfluff from vim directly (e.g., :!sqlfluff lint %) it does find the correct config; so it looks like vim "knows" the correct cwd, but I couldn't find any format strings in ale-command-format-strings that would give me that path.

@stale
Copy link

stale bot commented Oct 15, 2023

This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See :help ale-dev for information on writing tests.
If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

@stale stale bot added the stale PRs/Issues no longer valid label Oct 15, 2023
@w0rp w0rp removed the stale PRs/Issues no longer valid label Nov 19, 2023
Copy link

stale bot commented Mar 17, 2024

This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See :help ale-dev for information on writing tests.
If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

@stale stale bot added the stale PRs/Issues no longer valid label Mar 17, 2024
@stale stale bot closed this Apr 22, 2024
@w0rp w0rp reopened this Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PRs/Issues no longer valid
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement Suggestion][sqlfluff] Allow other config file other than .sqlfluff
4 participants