feat(lsp): will save #8952
feat(lsp): will save #8952
Conversation
matoous
force-pushed
the
md/lsp-will-save
branch
from
d339e29
to
89246f7
Compare
the-mikedavis
added
A-language-server
the-mikedavis
added
S-waiting-on-pr
matoous
force-pushed
the
md/lsp-will-save
branch
from
89246f7
to
6fb5979
Compare
the-mikedavis
added
S-waiting-on-review
|
@pascalkuthe rebased. I don't think #8949 affects this PR as this isn't covered under the file interest. |
helix-core/src/syntax.rs
Outdated
| // DocumentSynchronization | ||
| Save, | ||
| WillSave, |
There was a problem hiding this comment.
these should not be manually configurable, the fact athat we crruntly don't send these is a bug and there should be no option to turn this off (its not an optional part of the spec)
There was a problem hiding this comment.
Sorry, I am not sure if I follow. These are not configurable but used to request only LSPs support specific features. In this case we are checking for support of TextDocumentSyncOptions under https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_didRename.
There was a problem hiding this comment.
these are config options that allow users to disable/enable only certain LSP features. For convenience when also use these to check capabilities but you can check capabilities without this. You should only add something to this enum if you want the user to be able to turn an LSP feature off
helix-lsp/src/client.rs
Outdated
| pub fn text_document_will_save_wait_until( | ||
| &self, | ||
| text_document: lsp::TextDocumentIdentifier, | ||
| ) -> Option<impl Future<Output = Result<Vec<lsp::TextEdit>>>> { | ||
| let request = | ||
| self.call::<lsp::request::WillSaveWaitUntil>(lsp::WillSaveTextDocumentParams { | ||
| text_document, | ||
| reason: TextDocumentSaveReason::MANUAL, | ||
| }); | ||
|
|
||
| Some(async move { | ||
| let json = request.await?; | ||
| let response: Option<Vec<TextEdit>> = serde_json::from_value(json)?; | ||
| Ok(response.unwrap_or_default()) | ||
| }) | ||
| } |
There was a problem hiding this comment.
this should have a 1s timeout simiar to willrename
There was a problem hiding this comment.
We use 5s there, added the same here and extracted it into static DOCUMENT_OPS_TIMEOUT. I don't think it's worth it adding to config but having the same timeouts on all document-related calls seems sensible.
| // ------------------------------------------------------------------------------------------- | ||
| // Language features | ||
| // https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#languageFeatures | ||
| // ------------------------------------------------------------------------------------------- |
| @@ -870,6 +870,31 @@ impl Document { | |||
| // We encode the file according to the `Document`'s encoding. | |||
| let future = async move { | |||
| use tokio::{fs, fs::File}; | |||
|
|
|||
| if let Some(identifier) = &identifier { | |||
There was a problem hiding this comment.
this should definitly4 be handeled by the even t system and what I meant with my previous comments. We essentially want to keep LSP out of the main code and move it all behind events. So there should be a willsave event here and the lsp stuff should be moved to a hook. Same with didsave
matoous
force-pushed
the
md/lsp-will-save
branch
2 times, most recently
from
cf558bf
to
acf9986
Compare
matoous
force-pushed
the
md/lsp-will-save
branch
from
acf9986
to
03cc560
Compare
pascalkuthe
added
S-waiting-on-author
Implement
textDocument/willSavenotification1.Footnotes
https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_willSave ↩