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

Don't send textDocument/didSave unless server advertises support for it #164

Closed
DanTup opened this issue Mar 11, 2019 · 4 comments
Closed

Comments

@DanTup
Copy link
Contributor

DanTup commented Mar 11, 2019

I think the comment about didSave being sent unconditionally in #114 is incorrect. In the spec the ServerCapabilities has:

/**
 * Defines how text documents are synced. Is either a detailed structure defining each notification or
 * for backwards compatibility the TextDocumentSyncKind number. If omitted it defaults to `TextDocumentSyncKind.None`.
 */
textDocumentSync?: TextDocumentSyncOptions | number;

My interpretation is that textDocument/didSave should not be sent if textDocumentSync is 1, 2 or an object with .save == true. In Dart we're sending an object (TextDocumentSyncOptions) with save set to false.

@natebosch
Copy link
Owner

The save field should be a SaveOptions instance, which has a single boolean field includeText.

My impression is that the didSave request should be sent unconditionally, but that the server can indicate whether it wants the text included.

We could only send didSave notifications if the save field is a non-null dict, regardless of the value of includeText, but I'm worried it would be breaking and I can't tell from the spec whether that would be correct.

@natebosch
Copy link
Owner

Here's an issue discussing this:
microsoft/language-server-protocol#288

I think what you want to do in the server is send a dict without the save key, and I can interpret the missing key as "don't send".

@DanTup
Copy link
Contributor Author

DanTup commented Mar 13, 2019

Sorry, I made a mistake reading what we sent above where I mentioned a boolean (I read the willSave field). We are actually already not sending save:

new TextDocumentSyncOptions(
          true,
          TextDocumentSyncKind.Incremental,
          false,
          false,
          null,
        )
"textDocumentSync": {
	"openClose":  true,
	"change":  2,
	"willSave":  false,
	"willSaveWaitUntil": false
}

@natebosch
Copy link
Owner

Sounds good, this client doesn't support willSave so I'll see about disabling the didSave in the case where the save field is omitted.

natebosch added a commit that referenced this issue Mar 13, 2019
Towards #57
Towards #164

Add a capabilities library to filter down and parse out the options this
client cares about. Always store a dictionary of capabilities on the
server objects, and search those when checking for how other features
should behave.

This replaces the previous approach of storing a separate dictionary per
feature to track by filetype. Once we support multiple servers for a
given filetype the single config wouldn't make sense.
natebosch added a commit that referenced this issue Mar 13, 2019
Towards #57
Towards #164

Add a capabilities library to filter down and parse out the options this
client cares about. Always store a dictionary of capabilities on the
server objects, and search those when checking for how other features
should behave.

This replaces the previous approach of storing a separate dictionary per
feature to track by filetype. Once we support multiple servers for a
given filetype the single config wouldn't make sense.
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

No branches or pull requests

2 participants