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

[LSP] Do not send 'didSave' notification if language server does not support it #3207

Closed
bluz71 opened this issue Jun 14, 2020 · 5 comments · Fixed by #3930
Closed

[LSP] Do not send 'didSave' notification if language server does not support it #3207

bluz71 opened this issue Jun 14, 2020 · 5 comments · Fixed by #3930
Assignees

Comments

@bluz71
Copy link

bluz71 commented Jun 14, 2020

Spawning from the #3069 PR.

ALE when dealing with language servers appears to be sending the textDocument/didSave notification.

The Dart Analysis Server complains about that in the command line area after every write operation in Vim:

84124099-1ac5e700-aa7e-11ea-8b27-9dab565a68f1

Basically the Dart Analysis Server doesn't support that notification, hence it should not be sent. I think the server on initial handshake indicates what it does support and the client needs to take that into account.

That is clarified by Microsoft here in the LSP spec:

If present will save notifications are sent to the server. If omitted the notification should not be sent

Anyway, this warning is highly distracting and it should not be triggered.

This issue has been noted by other projects and been dealt with:

I would be nice if ALE can likewise fix this issue.

Thanks.

@hsanson
Copy link
Contributor

hsanson commented Oct 7, 2021

@hsanson
Copy link
Contributor

hsanson commented Oct 7, 2021

@bluz71 appreciated if you can test this PR #3930 and see if it solves this issue.

@bluz71
Copy link
Author

bluz71 commented Oct 9, 2021

@hsanson,

I switched over to native Neovim LSP early this year; so I am not in a position to test this out, sorry.

@akiross
Copy link

akiross commented Oct 14, 2021

I have the same issue, so I proceeded with testing #3930 and it seems to solve the issue. Thanks for the PR.

@hsanson
Copy link
Contributor

hsanson commented Oct 14, 2021

@akiross thanks for the testing. I have merged the fix now.

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

Successfully merging a pull request may close this issue.

3 participants