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

Return type of contentChanges in didChange does not appear to be correct #15

Open
slzatz opened this issue Oct 12, 2020 · 3 comments
Open
Assignees

Comments

@slzatz
Copy link

slzatz commented Oct 12, 2020

First of all, thank you for your work. I would still be reading the lsp spec without much comprehension if it wasn't for your client implementation. So I am new to pylspclient and lsp in general but believe there is an issue in lsp_client.py:

def didChange(...):
...

return self.lsp_endpoint.send_notification("textDocument/didChange", textDocument=textDocument, contentChanges=contentChanges) 

Couldn't get it to work until changed the return to:

return self.lsp_endpoint.send_notification("textDocument/didChange", textDocument=textDocument, contentChanges=[contentChanges]) 

(may need to scroll the above to the right - contentChanges needs to be in a list)

I find the msft lsp documentation a bit obtuse but assume that the ending [] in the definition below is saying that it needs to be in a list and when I made that change it worked.

contentChanges: TextDocumentContentChangeEvent[];
@yeger00 yeger00 self-assigned this Oct 13, 2020
@yeger00
Copy link
Owner

yeger00 commented Oct 13, 2020

Hi,
Thanks for opening the issue.

You are right that the function send_notification() receives a list. But there is no need to change contentChanges=[contentChanges] because the contentChanges parameter of the function didChange() is already a list.

Let me know if you think otherwise.

@slzatz
Copy link
Author

slzatz commented Oct 13, 2020

I am probably using the library incorrectly but what I am doing are things like:

lsp_client.didChange(pylspclient.lsp_structs.VersionedTextDocumentIdentifier(uri, 2),
                     pylspclient.lsp_structs.TextDocumentContentChangeEvent(range_, 1, "};"))

(where range_ is constructed from lsp_structs.Position as arguments to lsp_structs.Range. A complete aside but in a couple places you use the variable identifier range which will overwrite I believe the builtin range function but that is an unrelated issue.)

lsp_structs.TextDocumentContentChangeEvent(...) is a list according to the specification but it will produce a dictionary unless you do what I suggested or probably more sensible would be for lsp_structs.TextDocumentContentChangeEvent to return the list. Or am I doing something wrong (although with my change I can now successfully communicate with clangd).

@smathot
Copy link
Contributor

smathot commented Dec 28, 2020

@slzatz Each instance of TextDocumentChangeEvent corresponds to one contiguous change to document. Since it's possible that the document has been changed at multiple places, e.g. a bit at the start and a bit at the end, you can specify a list of them. So the function, as far as I can judge, is correct, but the way you call it is not, and should be like this:

lsp_client.didChange(pylspclient.lsp_structs.VersionedTextDocumentIdentifier(uri, 2),
                     [pylspclient.lsp_structs.TextDocumentContentChangeEvent(range_, 1, "};")])

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

3 participants