-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat(languages): add willSave and willSaveWaitUntil LSP support #14407
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
base: master
Are you sure you want to change the base?
Conversation
The implementation does not handle writing to new files properly, will try to fix this later this week. The problem seem to be the workspace edit opening the soon-to-be-written file in a duplicate buffer which results in all kinds of unintended behavior. |
The implementation now uses Transactions instead of WorkspaceEdits. Currently, the Transaction is applied regardless of whether the save succeeds, which (from what I’ve gathered) is the correct behavior. I consider the implementation complete unless there is further feedback on the code. |
&self, | ||
text_document: lsp::TextDocumentIdentifier, | ||
reason: lsp::TextDocumentSaveReason, | ||
) -> Option<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match other notification-style functions in the client let's not return anything here,
) -> Option<()> { | |
) { |
Similarly see text_document_did_open
|
||
match &capabilities.text_document_sync.as_ref()? { | ||
lsp::TextDocumentSyncCapability::Options(lsp::TextDocumentSyncOptions { | ||
will_save: enabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can match explicitly on will_save: Some(true)
here for the same effect as the if
below.
|
||
match &capabilities.text_document_sync.as_ref()? { | ||
lsp::TextDocumentSyncCapability::Options(lsp::TextDocumentSyncOptions { | ||
will_save_wait_until: enabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly here about pattern matching
let language_servers: Vec<_> = self | ||
.language_servers | ||
.iter_clients() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will send the will-saves to all language servers even if the document doesn't belong to a language server. Instead this can iterate over doc.language_servers.values()
else { | ||
continue; | ||
}; | ||
let edits = match helix_lsp::block_on(request) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will block the main thread and it could take a while if a language server sends edits when saving many documents with :write-all
. Instead these will-save requests should be done asynchronously like formatting requests
This PR adds support for the
willSave
andwillSaveWaitUntil
requests of the LSP.