Update "format document" and "format selection to use dbt-core-interface

[barrywhart]

Summary of changes:

• Add a status bar button that runs "Format document with sqlfluff"
• If dbt-core-interface is enabled in the extension
  • The "fix" command should use dbt-core-interface /format endpoint
  • The "format selection" action should also use dbt-core-interface /format endpoint

NOTE: I suggest reviewing the PR with "Hide whitespace" enabled, as some of the changes simply add if blocks around existing code.

The build is currently failing in the "Run tests" step for what appears to be an issue unrelated to my changes. Any insights? Log excerpt follows:

> rimraf out && tsc -p ./ && npm run postbuild

Error: node_modules/@types/glob/index.d.ts(29,42): error TS2694: Namespace '"/home/runner/work/vscode-sqlfluff/vscode-sqlfluff/node_modules/minimatch/dist/cjs/index"' has no exported member 'IOptions'.
Error: node_modules/@types/glob/index.d.ts(75,30): error TS27[24](https://github.com/sqlfluff/vscode-sqlfluff/actions/runs/7387815296/job/20097327298#step:7:25): '"/home/runner/work/vscode-sqlfluff/vscode-sqlfluff/node_modules/minimatch/dist/cjs/index"' has no exported member named 'IMinimatch'. Did you mean 'Minimatch'?
/home/runner/work/_actions/GabrielBB/xvfb-action/v1.2/node_modules/@actions/exec/lib/toolrunner.js:561

[BAntonellini]

Hi @RobertOstermann, is SQLFluff Format Selection a thing in vscode-sqlfluff nowadays? README says it's not

[RobertOstermann]

I think it is, the readme is just not updated. Not sure on that though

[noel]

I don’t think that works. We want to remove that option On Jan 5, 2024, at 11:21 AM, Robert Ostermann @.***> wrote: I think it is, the readme is just not updated. Not sure on that though

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

[RobertOstermann]

@noel I just tested this and formatting selection works when not using dbt interface (I didn't test with dbt) so the option should not be removed.

[noel]

@barrywhart I thought sqlfluff didnt support this. If it is just something not implemented in core interface then we keep it.

@BAntonellini Were you not able to hide the option when dbt interface is enabled?

[RobertOstermann]

@noel sqlfluff might not support this directly, but just passing in the selection text directly allows me to format the selection. I think it would be possible to hide the format selection option, but I'm not sure

[barrywhart]

@noel: Re: "Format selection", SQLFluff does not support it. It always formats or fixes an entire file (or a passed-in string). From what @RobertOstermann says, perhaps VSCode is passing the entire file as a string to SQLFluff, then determining which portion of the formatted result corresponds to the current selection, then updating only that portion in the editor. This is just guesswork; I don't know.

At the moment, it seems like we still don't understand the current behavior of vscode-sqlfluff and SQLFluff Format Selection without dbt-core-interface, and without knowing that, we aren't sure what to do with the dbt-core-interface behavior.

@noel, would you or @BAntonellini like to do some research and experimentation with SQLFluff Format Selection to determine if it's something we'd want to support with dbt-core-interface as well? Or would you like me to do that investigation? In order to support it, I think we'd need to change the new /format endpoint to accept a SQL string as input. Currently (based on our original Slack discussion), it only supports formatting a file, which would not work for SQLFluff Format Selection, because by definition that updates the entire file "in place", i.e. there's no opportunity for VSCode to do any magic slicing/extraction of the selected portion.

NOTE: As currently written, this PR removes SQLFluff Format Selection only in the case where dbt-core-interface is enabled. The PR summary had been wrong about that (it implied we were completely removing it). I corrected the PR description just now (no new code changes).

[RobertOstermann]

@barrywhart The format selection logic is handled within the rangeformat.ts file. Basically I get the text from the range that is selected and pass that text through to sqlfluff, which fixes the sql, and then I replace the selected range of text with the formatted text from sqlfluff. I added a bit of logic to handle some whitespace issues, but basically it is just having sqlfluff format a passed in string.

[barrywhart]

@RobertOstermann: Ok, so it should be possible to do the same with dbt-core-interface if we add support for passing in a string.

@noel: Do we want to do this?

[barrywhart]

@RobertOstermann: Can you take a look at this PR? Noel at Datacoves says it is working for them. The build is apparently failing for reasons unrelated to these changes.

[RobertOstermann]

@barrywhart It looks like this is some typescript types error and I would probably agree this is not related to this PR. I am not sure what would be causing the error. I am currently busy with other projects and do not have time to look into this closely, but I would imagine the fix is not too difficult.

[noel]

@BAntonellini would you be able to see if you can find the error?

[BAntonellini]

@barrywhart I've merged Master into your branch and compilation issues seem to be gone. could you check please?

➜  vscode-sqlfluff git:(bhart-add_sqlfluff_format) npm run compile 

> vscode-sqlfluff@3.0.0 compile
> rimraf out && tsc -p ./ && npm run postbuild

> vscode-sqlfluff@3.0.0 postbuild
> copyfiles "./test/suite/test_sql/**/*.sql" "./out" && copyfiles "./test/.sqlfluff" "./out"

[barrywhart]

@BAntonellini: It looks like the build is still failing. The MacOS build has this error:

/Users/runner/work/_actions/GabrielBB/xvfb-action/v1.2/node_modules/@actions/exec/lib/toolrunner.js:561
Error: node_modules/@types/glob/index.d.ts(29,42): error TS2694: Namespace '"/Users/runner/work/vscode-sqlfluff/vscode-sqlfluff/node_modules/minimatch/dist/cjs/index"' has no exported member 'IOptions'.
                error = new Error(`The process '${this.toolPath}' failed with exit code ${this.processExitCode}`);
Error: node_modules/@types/glob/index.d.ts(75,30): error TS2724: '"/Users/runner/work/vscode-sqlfluff/vscode-sqlfluff/node_modules/minimatch/dist/cjs/index"' has no exported member named 'IMinimatch'. Did you mean 'Minimatch'?

Not sure if this is the same error as before, but I remember it was a type-related error in code that I hadn't touched.

[BAntonellini]

Hey @RobertOstermann, CI is passing. This can now be merged.