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

Adds a lsp_save_all function #1876

Merged
merged 7 commits into from
Oct 25, 2021
Merged

Adds a lsp_save_all function #1876

merged 7 commits into from
Oct 25, 2021

Conversation

fserb
Copy link
Contributor

@fserb fserb commented Oct 20, 2021

We also fix open_file to avoid focusing the view if the file is already open.

This fixes #1849

plugin/save_command.py Outdated Show resolved Hide resolved
plugin/core/open.py Outdated Show resolved Hide resolved
boot.py Outdated Show resolved Hide resolved
plugin/core/open.py Outdated Show resolved Hide resolved
plugin/save_command.py Outdated Show resolved Hide resolved
plugin/save_command.py Outdated Show resolved Hide resolved
@fserb
Copy link
Contributor Author

fserb commented Oct 22, 2021

done :)

@fserb
Copy link
Contributor Author

fserb commented Oct 22, 2021

Fixed lint error.

@rchl
Copy link
Member

rchl commented Oct 22, 2021

I think there is a pretty fundamental issue with this approach.

This will trigger save actions for all changed files at once. And a save action is only allowed to run for a certain amount of time (by default 2s). If we start all the save actions for all files at the same time, there is a very high chance that many of those will time out since all actions will try to run at the same time but will be blocked by each other since all run in the same thread. Timing out means that the formatting on save or code actions on save might not get applies (the file will still be saved after the timeout).

I don't know how serious this issue would be in practice. It very much depends on the number of files to save and the servers used...

Possibly the solution here could be to increase the timeout per lsp_save command triggered. Maybe to even 10s.

@rchl
Copy link
Member

rchl commented Oct 22, 2021

With that said, if others are fine with that potential issue, I'm not gonna block this from being integrated. It could be improved later.

Though for the purpose of visibility, I'd ask you to also add the new command to the default key bindings, commented out.

@fserb
Copy link
Contributor Author

fserb commented Oct 22, 2021

Added keymap.

Default.sublime-keymap Outdated Show resolved Hide resolved
@rwols
Copy link
Member

rwols commented Oct 25, 2021

Is anyone against merging this?

@rwols rwols merged commit 61fd9a7 into sublimelsp:main Oct 25, 2021
@fserb fserb deleted the saveall branch October 25, 2021 19:48
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.

lsp_save_all or a way to enable lsp_code_actions_on_save for save_all
5 participants