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): don't assume ansi dialect and require config #519

Merged
merged 1 commit into from
Aug 16, 2024
Merged

fix(sqlfluff): don't assume ansi dialect and require config #519

merged 1 commit into from
Aug 16, 2024

Conversation

igorlfs
Copy link
Contributor

@igorlfs igorlfs commented Aug 11, 2024

Assuming the ANSI dialect may lead to parsing errors when using syntax specific to some dialect, so it shouldn't be the default. Therefore, a config is mandatory, to select the dialect.

We can't just update the exit codes because sqlfluff straight up refuses to format with some erros. For instance, I was getting:

WARNING    Fixes for LT09 not applied, as it would result in an unparsable file. Please report this as a bug with a minimal query which demonstrates this warning.                                                                             
WARNING    One fix for LT09 not applied, it would re-cause the same error.                                                                                                                                                                     

When running sqlfluff fix --dialect=ansi - for a given query (on the command line).

@github-actions github-actions bot requested a review from stevearc August 11, 2024 03:30
@stevearc
Copy link
Owner

I agree it seems like a better default to not force the dialect. Some people may still want it to function even when a config file isn't present, which isn't a blocking issue but for anyone that wants that feature and finds this comment, I would review a PR that adds the dialect arg when a config file is not found.

@stevearc stevearc merged commit bb10949 into stevearc:master Aug 16, 2024
8 checks passed
@LJFRIESE
Copy link

Glad to see this change - It threw me when I first was trying to use sqlfluff.

I am currently trying to work out a way to have conform capture some of the more specific warnings/errors that sqlfluff provides. Specifically, when sqlfluff encounters an unparsable section during a fix, conform errors out without supplying the line number of the section. The sqlfluff cmd DOES supply this information, but it's part of a wall of text that gets printed, rather than a simple err message, so it's not getting logged in conform unless log level = DEBUG.

I've been exploring ways of intercepting messages from the vim.fn.jobstart, and also looking to see if conform's internal logging was filtering anything, but I've not found anything yet.

I'm happy to put in time to work on this if there is a direction you could point me in.

@stevearc
Copy link
Owner

This is something that conform is not really set up to do. We support formatting, there are not really any affordances for propagating other information upwards. Honestly I'd recommend trying to get sqlfluff (or some other SQL linter) to run with nvim-lint.

@Zerohertz
Copy link

I agree it seems like a better default to not force the dialect. Some people may still want it to function even when a config file isn't present, which isn't a blocking issue but for anyone that wants that feature and finds this comment, I would review a PR that adds the dialect arg when a config file is not found.

Hello @stevearc,

I wanted to take a moment to express my gratitude for your work. I'm currently using conform.nvim through LazyVim, and it's been very helpful.

After a recent update, however, I encountered an issue where the automatic formatting with sqlfluff was not functioning as expected. It took me a bit of time to figure it out because the following log was not initially appearing in the ConformInfo:

Log file: /home/zerohertz/.local/state/nvim/conform.log
          User Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects:
          ansi, athena, bigquery, clickhouse, databricks, db2, duckdb, exasol, greenplum, hive, mariadb, materialize, mysql, oracle, postgres, redshift, snowflake, soql, sparksql, sqlite, teradata, trino, tsql, vertica

It might be helpful to mention somewhere that a .sqlfluff config file in the current working directory is required for it to work properly. I believe this would make it easier for others who might run into the same problem.

Once again, thank you for your great work!

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.

4 participants