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

SQL formatting #4

Closed
MrLoh opened this issue Jan 29, 2020 · 28 comments · Fixed by #14
Closed

SQL formatting #4

MrLoh opened this issue Jan 29, 2020 · 28 comments · Fixed by #14
Labels
enhancement New feature or request

Comments

@MrLoh
Copy link

MrLoh commented Jan 29, 2020

Have you ever looked into adding sql formatting to this library, prettier has spoiled me and there seem to be sql formatting extensions for vscode, but I have no idea, what would be needed to build a formatter for tagged template literals.

@frigus02
Copy link
Owner

I did look into SQL formatting briefly. However I could not find any good tool/library/extension for it.

I think for this library the way to do it, would be to tell VS Code about the language in the SQL template literals. It's quite easy to achieve this for syntax highlighting. And it's quite a bit harder for IntelliSense and formatting support. The typescript-lit-html-plugin supports this for html tagged template literals. Something similar would be quite nice for this library. I think that this would enable all existing VS Code SQL formatting extensions to work in sql tagged template literals.

I don't have time to work on this at the moment. But it would be amazing if you or anyone else would like to work on this.

@frigus02 frigus02 added the enhancement New feature or request label Jan 31, 2020
@frigus02 frigus02 changed the title sql formatting SQL formatting Apr 28, 2020
@karlhorky
Copy link

Maybe https://www.npmjs.com/package/pg-formatter would help, if you're ok with requiring Perl (because the library wraps pgFormatter).

This would be amazing to have this!

@frigus02
Copy link
Owner

I don't think I mind requiring Perl, at least if it's optional.

I'm not entirely sure how formatting in general should be handled, though. My feeling is that it would be nice for formatting to be a separate extension. If this extension could mark the SQL text properly as SQL, it might be formatted automatically by any extension that supports formatting of SQL. I think I read somewhere that something like this is possible. But I can't remember where. But I could also be wrong, in which case this might be the correct place.

@karlhorky
Copy link

My feeling is that it would be nice for formatting to be a separate extension

Yeah, what I would really like would be for it to be done by Prettier, which is not a new idea, but also probably won't be in core:

There is a first fledgling attempt at a plugin (https://github.com/benjie/prettier-plugin-pg), but it's probably going to be a while.

As for non-Prettier options, there is an ESLint plugin, which currently gets the closest (using the format rule with --fix), but I'm personally not a big fan of always running ESLint auto-fixing in my editor every time I save:

https://github.com/gajus/eslint-plugin-sql

@frigus02
Copy link
Owner

Hm okay. After reading a bit more, I think formatting SQL in tagged template literals may have to be this plugin's responsibility. We have to implement:

https://github.com/microsoft/typescript-template-language-service-decorator/blob/8c06f84f1cc368a80df85faea3896c0b2515a590/src/template-language-service.ts#L34-L39

There is a VS Code extension, which uses pgFormatter: https://github.com/bradymholt/vscode-pgFormatter. It might be useful here.

@karlhorky
Copy link

karlhorky commented Nov 22, 2020

After reading a bit more, I think formatting SQL in tagged template literals may have to be this plugin's responsibility. We have to implement

Nice! That would make this plugin the single solution for everything - that would be awesome.

From my research into how a VS Code extension could implement something similar, I ran across the codeActionsOnSave setting, which leads me to believe that an extension that would be a vscode.CodeActionProvider such as in the official sample from the VS Code team would be able to be configured like this (and the order can also be chosen in settings.json). A second example of an extension that provides Code Actions.

There is a VS Code extension, which uses pgFormatter

Ah super interesting, I somehow missed this one! I would much prefer it being in your extension, but I have also opened an issue over there to see how willing the author is to support formatting of SQL in template strings: bradymholt/vscode-pgFormatter#40

Edit: Oh, maybe the author's not open to this after all: bradymholt/vscode-pgFormatter#40 (comment)

@frigus02
Copy link
Owner

frigus02 commented Nov 22, 2020

I thought it would work like this, but I can't figure out when getFormattingEditsForRange is called:

https://github.com/frigus02/typescript-sql-tagged-template-plugin/compare/formatting#diff-b0c4ada88752efa437a29f5186c2ea2fd911c5a022cd99a0cb6f1b2dc071fcd4R236

Need to play with it a bit more.

@karlhorky
Copy link

karlhorky commented Nov 22, 2020

Hm... yeah looking around a bit more, I couldn't find much on that method. But I did find something on DocumentRangeFormattingEditProvider, maybe that's related?

Eg. something like this: https://github.com/mfeckie/handlebars-formatter/blob/345f08f5d33f0347dc57dbf47698f3dcae02e517/src/extension.ts#L37-L78

@frigus02
Copy link
Owner

Yeah, I think those are VS Code APIs, which we could use in vscode-sql-tagged-template-literals.

It would be nice if we could find a way to implement this in the TypeScript language server plugin, though. Because then it would be independent of the editor and could work everywhere.

You can test this with this repo and VS Code without any extensions:

  1. Install dependencies and compile (yarn && yarn compile)
  2. Change this path to where you cloned the repository:
    "name": "/Users/jkuehle/Projects/typescript-sql-tagged-template-plugin/build",
  3. Open the folder test-project in VS Code and open the index.ts file. It should load the plugin and show some errors.

All this works without an extension. Ideally formatting would work, too.

@frigus02
Copy link
Owner

frigus02 commented Nov 22, 2020

I'm still trying to understand why getFormattingEditsForRange isn't called. I also tried to decorate getFormattingEditsForDocument with similar results. Sadly the Writing-a-Language-Service-Plugin docs don't mention formatting.

Started to dig around a bit. The TypeScript extension calls "format" here (which is registered using registerDocumentRangeFormattingEditProvider):
https://github.com/microsoft/vscode/blob/986e1248f6d8c1aa2a7f57a3fadbb00f94248c2b/extensions/typescript-language-features/src/languageFeatures/formatting.ts#L34

I think this calls the TypeScript server and I can see a command with the same name:
https://github.com/microsoft/TypeScript/blob/9ff24b6fc87dd4a376a434d1019d356dfe743c53/lib/protocol.d.ts#L23

And I think that's mapped to getFormattingEditsForRange:
https://github.com/microsoft/TypeScript/blob/79ffd03f8b73010fa03cef624e5f1770bc9c975b/src/server/session.ts#L2642-L2644

@karlhorky
Copy link

It would be nice if we could find a way to implement this in the TypeScript language server plugin, though. Because then it would be independent of the editor and could work everywhere

Oh wow, low level magic. Yeah, I don't have much experience with extension development alone, let alone the TS language server :)

Although, looking about I did see these two resources which may shed some light on what's going on:

@karlhorky
Copy link

You can test this with this repo and VS Code without any extensions

Right, just tried this out for a few hours, and came to the same conclusions as in your last comment. It seems like it should be called, but it is not being called...

I set it up with a logfile using:

export TSS_LOG="-level verbose -file /Users/karl/projects/typescript-sql-tagged-template-plugin/tss.log"

And it actually logs out the command:

Info 44   [2:57:57.976] request:
    {"seq":10,"type":"request","command":"format","arguments":{"file":"/Users/karl/projects/typescript-sql-tagged-template-plugin/test-project/index.ts","line":48,"offset":2,"endLine":48,"endOffset":50}}
Perf 45   [2:57:57.978] 10::format: elapsed time (in milliseconds) 2.3522
Info 46   [2:57:57.979] response:
    {"seq":0,"type":"response","command":"format","request_seq":10,"success":true,"body":[]}

But no logging of the format requested. config: message... hm... 🤔

@karlhorky
Copy link

Putting a logger.info inside of the TS Server Session.prototype.getFormattingEditsForRange does seem to result in lines in the logs (I did this by using the local version of TypeScript and modifying typescript/lib/tsserver.js:

Session.prototype.getFormattingEditsForRange = function (args) {
  var _this = this;
  this.logger.info('getFormattingEditsforRange in session');

@karlhorky
Copy link

karlhorky commented Nov 23, 2020

Ok, I think I may have narrowed it down a bit. It seems that the proxy for getFormattingEditsForRange is not getting called. If you add a this._logger = _logger; to the constructor and then a this._logger.log('in proxy: ' + property) to the getter at the following link, you will not see a log for getFormattingEditsForRange - the wrapped function is not being called.

https://github.com/microsoft/typescript-template-language-service-decorator/blob/3af3ba3527f8919c1c8a4fbe8348206585177ec4/src/template-language-service-decorator.ts#L47-L51

It is wrapped here:

https://github.com/microsoft/typescript-template-language-service-decorator/blob/3af3ba3527f8919c1c8a4fbe8348206585177ec4/src/template-language-service-decorator.ts#L137-L163

cc @mjbvz is this a bug in typescript-template-language-service-decorator? Or maybe using getFormattingEditsForRange isn't possible anymore in Language Server Plugins with newer TypeScript versions?

@mjbvz
Copy link

mjbvz commented Nov 24, 2020

The lit-html TS plugin's formatting still works for me with TS 4.1. I believe it's using the same function

@karlhorky
Copy link

Do you mean mjbvz/vscode-lit-html? It doesn't seem to be using the getFormattingEditsForX APIs:

Doesn't it work with the built-in formatting in VS Code using the embedded meta.embedded.block.html?

It's different than this plugin, which actually contributes formatting edits from a third-party formatting engine.

@frigus02
Copy link
Owner

The lit-html extension is split in VS Code extension and TypeScript server plugin, too. You can see getFormattingEditsForRange here:

I'm going to play a bit with the lit-html plugin and figure out what the difference is.

@frigus02
Copy link
Owner

frigus02 commented Nov 24, 2020

Okay, formatting with the typescript-lit-html-plugin works for me in Vim (using coc.nvim and the coc-tsserver extension). However it doesn't work in VS Code. I think VS Code my not invoke the format command of the language server. That would be really sad.

Edit: This comment (not sure how reliable that information is) says VS Code doesn't use the language server protocol for TypeScript. This could explain why it works in Vim, but not VS Code.

At least that's another thing to look at. Thanks, Matt.

@frigus02
Copy link
Owner

Ha, SQL formatting works in Vim with the current formatting branch. Requires Perl, obviously.

Now, how do we make it work in VS Code? I hope this doesn't require a special extension.

@karlhorky
Copy link

So, after thinking about it for a day, my gut feel hypothesis was this:

  1. languageService.getFormattingEditsForRange in /src/server/session.ts is being called after all

I think this may be true because when I added a this.logger.info() call one line above, it worked (as I noted above in my comment in #4 (comment))

  1. However, languageService.getFormattingEditsForRange does not refer to the proxy wrapper function created in the library.

I think this because the this._logger.log() does not show anything when called inside this function (but it does work in other wrapped proxy functions).

This line of reasoning (although just a gut feel) leads me to the hypothesis that the getFormattingEditsForRange proxy with the wrapper function is not being applied properly by the library. Either A) it is applied and then overwritten somewhere else in the code, or B) it is applied after a reference to the previous function is saved.

But maybe it's way off base, not sure. It's the only way I could think of why there would be a discrepancy here (this as a beginner with the TypeScript codebase and language servers, of course).

@frigus02
Copy link
Owner

I found it 🎉

Apparently VS Code starts 2 different language servers: one for syntax and one for semantics. The semantic one seems to be the one that loads plugins. But here the "format" command is configured to always run on the syntax server:

https://github.com/microsoft/vscode/blob/72f4a2a7deec61c8c7439ed8db1155b2c1a4551c/extensions/typescript-language-features/src/tsServer/server.ts#L481

If I cheekily change that in my VS Code installation (under %localappdata%\Programs\Microsoft VS Code\resources\app\extensions\typescript-language-features\dist\extension.js), then formatting works fine. I think I'm going to open an issue in the VS Code repository and ask for advice.

@karlhorky
Copy link

Nice, really great investigative work @frigus02 !! Diving really deep :) 💯

@frigus02
Copy link
Owner

Oh, there is even a setting for this:

TypeScript › Tsserver: Use Separate Syntax Server

  • With this turned on (was on by default for me), formatting does not call plugins
  • With this turned off, formatting does call plugins and SQL formatting works

Now I also understand the log files.

  • The VS Code command "TypeScript: Open TS Server log" always opens the log file of the semantic server. It contains logs like:

    Info 29   [12:48:42.489] Enabling plugin C:\code\typescript-sql-tagged-template-plugin\build ...
    Info 30   [12:48:42.489] Loading C:\code\typescript-sql-tagged-template-plugin\build ...
    Info 31   [12:48:42.844] [typescript-sql-tagged-template-plugin] new config: ...
    Info 33   [12:48:42.846] Plugin validation succeded
    

    But it doesn't contain any format related logs.

  • The log file of the syntax server does contain format related logs. But it doesn't seem to load the project config and therefore it also doesn't load plugins.

The syntax server logs do load global plugins, though. This makes me think SQL formatting should work if the plugin is activated by the VS Code extension, rather than by the tsconfig.json file. I need to give this a go.

@frigus02 frigus02 mentioned this issue Nov 25, 2020
4 tasks
@frigus02
Copy link
Owner

The extension indeed causes the plugin to be loaded in both the semantic and syntax server.

Unfortunately the syntax server does not receive the configurePlugin command, because it's marked as a semantic command: https://github.com/microsoft/vscode/blob/50cc1d0e9777f3f76f460c24feb139ca695ff506/extensions/typescript-language-features/src/tsServer/server.ts#L493

This means formatting has to be turned on by default. I guess we need to build do some feature detection (maybe check if Perl is install and in the PATH). This doesn't sound like a bad idea anyway.

@frigus02
Copy link
Owner

I think all major issues are solved. I'm going to track the remaining work on the PR 🙂

@frigus02
Copy link
Owner

I pushed version 0.1.0 of this plugin, which supports formatting. Thanks again @karlhorky for all your help.

I also uploaded version 0.0.16 of the VS Code, which now supports formatting. See changelog for more details:

https://github.com/frigus02/vscode-sql-tagged-template-literals/blob/master/extension/CHANGELOG.md#0014---2020-10-21

@karlhorky
Copy link

karlhorky commented Nov 28, 2020

So amazing, really impressive work @frigus02 !! 🔥

Just giving it a spin, seems to be working!! I'm going to need to probably pass some additional configuration options to pgFormatter such as --comma-break (see screenshots below), but I suppose that will require:

  1. Passing the option(s) along through gajus/pg-formatter, à la your PR at Add "tabs" and "noRcFile" options gajus/pg-formatter#7
  2. Setting this option in the typescript-sql-tagged-template-plugin and vscode-sql-tagged-template-literals (wonder if --comma-break should be even the default)

INSERT without --comma-break

Screen Shot 2020-11-28 at 16 32 09

INSERT with --comma-break

Screen Shot 2020-11-28 at 16 34 18

Screenshot source: http://sqlformat.darold.net/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants