search

eclipse / lsp4e

Language Server Protocol support in Eclipse IDE
Eclipse Public License 2.0
57 stars 54 forks source link

Register PublishDiagnosticsCapabilities #1008

Closed zulus closed 1 month ago

zulus commented 1 month ago

Simple fix for #1007

rubenporras commented 1 month ago

thanks

akurtakov commented 1 month ago

As this is prereq for Typescript update in WildWebDeveloper is there a chance for a quick release for this one?

rubenporras commented 1 month ago

Yes, no problem

akurtakov commented 1 month ago

Let me push the lsp4j and guava updates first.

rubenporras commented 1 month ago

Thanks for pointing that out. I had plan to wait for https://github.com/eclipse/lsp4e/pull/1010. That addresses LSP4J and Guava, or is any other PR coming?

akurtakov commented 1 month ago

1010 addresses LSP4J. The Guava one is trickier as we use it from Orbit and if we update it that would lead to other deps (bouncycastle, icu4j, objectweb-asm) from 2024-06 to be used or we can point to the maven artifact directly (like done here). Please have a say which one of the 3 options below you prefere:

rubenporras commented 1 month ago

To keep the target file simple, I would keep using the orbit repository, if that is possible. Maybe it is not such a big deal to just update the orbit repo?

rubenporras commented 1 month ago

The other option is to remove Guava, it does not look like we use for much, and we had a PR in the past where we tried to do the removal.

akurtakov commented 1 month ago

Here you are https://github.com/eclipse/lsp4e/pull/1011

zulus commented 1 month ago

in general would be nice to check if all capabilities that are already supported by lsp4e are correctly registered in SupportedFeatures.java

mickaelistria commented 1 month ago

FWIW, this capability is new and wasn't required to get publishDiagnostics working, the behavior of the typescript language server isn't really backward compatible here, although the spec is (and that's why it has been the only LS failing). But anyway, I agree with you that review capabilities often could be useful.

zulus commented 1 month ago

FWIW, this capability is new and wasn't required to get publishDiagnostics working, the behavior of the typescript language server isn't really backward compatible here, although the spec is (and that's why it has been the only LS failing). But anyway, I agree with you that review capabilities often could be useful.

yeah, which is funny because even if capability isn't available ts calculate diagnostics TS calculate this, but since 4.1 or 4.2 before send message they just check capability (i found this directly in source code, probably someone requested this)

I don't know since when LSP have publishDiagnosticsCapabilities (I don't see @since is spec), probably most LS just send them