search

eclipse-lsp4j / lsp4j

A Java implementation of the language server protocol intended to be consumed by tools and language servers implemented in Java.
https://eclipse.org/lsp4j
Other
581 stars 141 forks source link

Why is `WorkspaceEditCapabilities`'s constructor setting `documentChanges` is deprecated? #822

Closed ForNeVeR closed 1 month ago

ForNeVeR commented 3 months ago

See this constructor: https://github.com/eclipse-lsp4j/lsp4j/blob/cc20058d5b3d58596d49b71ca3c20308cc451844/org.eclipse.lsp4j/src/main/java/org/eclipse/lsp4j/Protocol.xtend#L107-L110

It is unclear why it was deprecated. This deprecation notice has been added in commit 43a0e07afa8e1a385af77e2a90f60dfa06b5c402, seemingly after a review in #277 and deprecation of Boolean resourceChanges member of the same type.

Perhaps @yaohaizh just mistaken resourceChanges and documentChanges while doing that? Anyway, I think it's a good idea to add an explanation to the deprecation notice (it looks like you can just safely use setDocumentChanges instead). Optionally, we may un-deprecate the constructor in question.

pisv commented 3 months ago

Although it is indeed unclear why this constructor was deprecated, I'd be inclined to just remove it now. There are as many as five properties in WorkspaceEditCapabilities -- all of them are optional, and there seems to be nothing special in the documentChanges property to justify this constructor.

@jonahgraham WDYT?

ForNeVeR commented 3 months ago

If you don't wanna to introduce a breaking change for nothing, then it would be better to maybe start by providing a description in the deprecation annotation 😅

jonahgraham commented 1 month ago

I added some documentation based on this conversation. We can remove the deprecated code at some point, but I recommend we do that around the time we update to soon to be released (I assume?) LSP 3.18 spec.