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

Add support for SnippetTextEdit #847

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hopehadfield
Copy link

Fixes #810

One note is that this implementation violates the spec in that SnippetTextEdit extends TextEdit. Given that the edits field of TextDocumentEdit is defined as a List<TextEdit>, this was the only way to include the appropriate support for SnippetTextEdit in TextDocumentEdit without making this a breaking change (i.e. by defining edits as a List<Either<TextEdit,SnippetTextEdit>>).

@pisv
Copy link
Contributor

pisv commented Sep 26, 2024

@hopehadfield Thank you for your contribution!

Here are my remarks:

  • StringValue.kind needs to be an instance field, not a static one.

A separate class, StringValueKind, should define the possible values for StringValue.kind, similarly to e.g. ResourceOperationKind.

  • TextDocumentEdit.edits needs to be annotated with @JsonAdapter indicating a type adapter that would create instances of TextEdit, AnnotatedTextEdit, or SnippetTextEdit as appropriate based on the actual JSON data.

Without such a type adapter, only basic TextEdits would ever be created when deserializing a text document edit. (I guess it does mean that we don't yet have complete support for annotated text edits in LSP4J.)

It can be verified by adding some tests for deserialization of text document edits containing different kinds of text edits. I'd suggest taking a look at JsonParseTest.testRenameResponse3 as a starting point for adding such tests.

  • This can be discussed, but I'd rather be in favor of not making SnippetTextEdit artificially extend TextEdit.

Although your PR as currently implemented does avoid introducing a breaking change, there are also some concerns:

  1. The JSON shape of SnippetTextEdit extending TextEdit doesn't match the spec because of the extraneous newText property.

  2. Any code that processes TextEdit objects in a polymorphic way (i.e. without using instanceof checks) would process instances of SnippetTextEdit extending TextEdit as remove edits, since their newText is always empty.

In general, LSP4J tries to match the specification as close as possible, even if breaking changes would need to be introduced. Unfortunately, there is a significant difference between a nominal type system in Java and a structural one in TypeScript, which means that, in general, some breaking changes in LSP4J are unavoidable when a new version of the LSP spec is released.

  • Since LSP 3.18 has not been released yet, I think that the new protocol elements need to have the Beta annotation on them.

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

Successfully merging this pull request may close these issues.

Support for SnippetTextEdit
2 participants