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

Always send a did save notification #3957

Closed
wants to merge 2 commits into from
Closed

Always send a did save notification #3957

wants to merge 2 commits into from

Conversation

Techatrix
Copy link

For some reason Helix only sends the textDocument/didSave notification if the includeText option is set to true.

@kirawi kirawi added A-language-server Area: Language server client S-waiting-on-review Status: Awaiting review from a maintainer. C-bug Category: This is a bug labels Sep 25, 2022
@the-mikedavis
Copy link
Member

The include_text option can be false and the didSave will still be sent; it just won't include the full document text. If I'm reading the old code right, we don't send didSave when the server capability is None or Some(TextDocumentSyncSaveOptions::Supported(false)) which I think is correct given the spec's wording, no?

@Techatrix
Copy link
Author

What is TextDocumentSyncSaveOptions::Supported(false) trying to represent? It doesn't directly map to anything in the specs if i'm not mistaken.
After re-reading the specs i see that only the textDocument/didOpen, textDocument/didChange and textDocument/didClose notifications are mandatory and textDocument/didSave therefore isn't. The server may send a client/registerCapability to indicate that they are interested in the textDocument/didSave notification but it doesn't seem to be supported. I always see
Language Server: Method not found client/registerCapability in the logs. Helix's initialize request also doesn't specify TextDocumentSyncClientCapabilities so its not even possible to find out if helix supports sending textDocument/didSave.

There is nothing stopping the client from always sending this notification which is also done by other clients (VS Code & Sublime Text).

@the-mikedavis
Copy link
Member

It's the line here: https://github.com/microsoft/language-server-protocol/blob/697910c7c3c559eafdd98fa8f3c001bee1ee7fdc/_specifications/lsp/3.17/textDocument/didSave.md?plain=1#L13 when the boolean is false

Helix's initialize request also doesn't specify TextDocumentSyncClientCapabilities

That should definitely be added 👍 (here)

@Techatrix
Copy link
Author

I now see what my mistake was. I was looking at the incomplete version of TextDocumentSyncOptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-server Area: Language server client C-bug Category: This is a bug S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants