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

Format Document on save option #8

Closed
rwols opened this issue Aug 16, 2017 · 18 comments
Closed

Format Document on save option #8

rwols opened this issue Aug 16, 2017 · 18 comments

Comments

@rwols
Copy link
Member

rwols commented Aug 16, 2017

Similar to Clang Format, it'd be nice to have a setting in the settings (false by default maybe) called "format_on_save" that will apply the "Format Document" action when the view is saved (on_pre_save)

EDIT: Since it's probably an async option you'd have to bake in some way to "wait" for the server to give a response before actually returning from EventListener.on_pre_save. I'll stop thinking about details now.

@tomv564
Copy link
Contributor

tomv564 commented Aug 17, 2017

Thanks for the idea and implementation hints.

Blocking in on_pre_save would let us format before the user's save, but I'm a bit worried about the user experience and error handling in this case.

Perhaps two saves is okay?

  1. User saves
  2. on_pre_save_async: request textDocument/formatting
  3. document saved
  4. LSP edits received
  5. apply edits
  6. set 'no_format' flag on view
  7. view.run_command('save')
  8. on_pre_save_async: do nothing because 'no_format' flag
  9. formatted document saved (if dirty)
  10. clear 'no_format' flag on view

@rwols
Copy link
Member Author

rwols commented Aug 17, 2017

How can you be sure step 4 is executed after step 3? Maybe the server sends back a response before the document is even saved?

@tomv564
Copy link
Contributor

tomv564 commented Aug 17, 2017

Good point, some experimentation is needed!
I see that https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#new-willsavewaituntiltextdocument-request is the intended LSP mechanism for format_on_save

@cixtor
Copy link

cixtor commented Aug 30, 2018

Good point, some experimentation is needed! I see that https://github.com/Microsoft/language-server-protocol/blob/master/protocol.md#new-willsavewaituntiltextdocument-request is the intended LSP mechanism for format_on_save

That link is broken, the new documentation for LSP —and more specifically for this method— is here:

https://microsoft.github.io/language-server-protocol/specification#textDocument_willSaveWaitUntil

@karolyi
Copy link

karolyi commented Feb 26, 2019

+1, voting for this. we need format_on_save

@oralordos
Copy link

I am currently working on a implementation of this. I decided to use a blocking format backed by a timeout. If the timeout expires, it will just save and ignore the formatting. Mostly working, just need to cleanup the code and add some settings so it is not always on with a hard-coded timeout. I also need to implement the willSaveWaitUntil for the language servers that support it, instead of a standard format document command.

You can find what I have so far here: https://github.com/oralordos/LSP

@ayoub-benali
Copy link
Contributor

@oralordos looking forward to testing it :)

@haferburg
Copy link
Contributor

haferburg commented Apr 7, 2019

@oralordos I'm testing your changes with my very barebones server that doesn't handle any text synchronization yet. The only capability it advertises is documentFormattingProvider. In order to do the formatting, it reads the file from disk. That means the file already needs to be saved, otherwise the server would generate text edits for an outdated version.

I don't know if this is a scenario that you actually want to support, but the format-on-save feature doesn't work properly in this case. Then again, neither does the regular LSP: Format Document command from the palette, so I don't know whether this even belongs here, or warrants a separate issue.

To support this scenario, the client could detect it, then listen to on_post_save instead, and save again afterwards. Alternativaly, the client should not only check for the documentFormattingProvider capability, but also textDocumentSync (which defaults to TextDocumentSyncKind.None) in FormatOnSave.on_pre_save.

@rwols
Copy link
Member Author

rwols commented Sep 2, 2019

Having much more experience with LSP now I think what I wanted to expres is that this text editor client should implement textDocument/willSaveWaitUntil, and then a language server should decide what to do with that notification. It could format the document, or it could do something else. Perhaps set via workspace/configuration or initializationOptions or via a plain command-line option.

But I disagree with my past self now in that we should offer a "format on save" option; all we can do is enable willSaveWaitUntil and then let the language server decide.

@tomv564
Copy link
Contributor

tomv564 commented Sep 3, 2019

Some servers (Metals, RLS) rely on the editor (eg. VS Code's editor.formatOnSave feature) to invoke the formatter after willSave. I think few language servers will go the additional effort to implement their own internal willSave->format hook if their biggest customer does it for them.

Gopls even leans on VS Code to run code actions (https://github.com/golang/go/wiki/gopls#vscode) instead of doing it in willSaveWaitUntil.

What do you think of implementing the "format_on_save" feature in LSP by blocking in on_pre_save only (instead of baking blocking into the rpc client) ? Would that work?

@tomv564 tomv564 mentioned this issue Sep 25, 2019
@tomv564
Copy link
Contributor

tomv564 commented Sep 30, 2019

Two suggestions from this issue are now implemented:

Running "Format Document" on save is enabled by adding a lsp_format_on_save set to true in your Sublime settings (not LSP Package settings!).

The willSaveWaitUntil request (as used by vscode's eslint server) is always sent by LSP, your server likely has a setting (eg. "autoFixOnSave": true) that enables formatting actions for it.

@malthejorgensen
Copy link

For anyone coming here in 2024, it seems that you must set "lsp_format_on_save": true at the top-level of the LSP.sublime-settings for auto-format on save to take effect.

@photex
Copy link

photex commented Jan 14, 2024

I've tried to put this setting anywhere I could find and it has not yet worked. Manually running the lsp format command from the palette works at least.

@philippotto
Copy link
Contributor

For anyone coming here in 2024, it seems that you must set "lsp_format_on_save": true at the top-level of the LSP.sublime-settings for auto-format on save to take effect.

Just to clarify: it should not be in the LSP-specific settings, but in your sublime settings. Quoting @tomv564:

Running "Format Document" on save is enabled by adding a lsp_format_on_save set to true in your Sublime settings (not LSP Package settings!).

@predragnikolic
Copy link
Member

Just to clarify: it should not be in the LSP-specific settings

That is not the current behavior.

The current behavior now is that
lsp_format_on_save will work in:

  • LSP.sublime-settings
  • Preference.sublime-settings
  • in sublime-project files

The same apples to other LSP global settings like lsp_format_on_paste and lsp_code_actions_on_save

https://lsp.sublimetext.io/client_configuration/#per-project-overrides

@philippotto
Copy link
Contributor

That is not the current behavior.

The current behavior now is that lsp_format_on_save will work in:

Hm, I cannot confirm this on my setup. I recorded a short video where I enable lsp_format_on_save in LSP.sublime-settings and hit ctrl+s repeatedly in a TS file. Nothing happens then. Afterwards, I activate the setting in the global settings, which works then.

format_on_save.mp4

Not sure what is going on. I'm not really disturbed by this, but wanted to mention it. I'm LSP 1.28.0 by the way.

@predragnikolic
Copy link
Member

If you have lsp_format_on_save set to false in Preference.sublime-settings,
that will have a higher priority than lsp_format_on_save set to true in LSP.sublime-settings file.

enabled = view_format_on_save if isinstance(view_format_on_save, bool) else userprefs().lsp_format_on_save

@philippotto
Copy link
Contributor

If you have lsp_format_on_save set to false in Preference.sublime-settings,
that will have a higher priority than lsp_format_on_save set to true in LSP.sublime-settings file.

Ok, I got it now. I didn't have lsp_format_on_save set anywhere else than LSP.sublime-settings, but: I had a LSP block in my project-settings. Apparently, that block is not merged with the "global" LSP settings. Since the LSP block didn't contain a value for lsp_format_on_save, the default of False was used. Removing the LSP block from the project settings and restarting sublime fixed this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests