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

Register PublishDiagnosticsCapabilities #1008

Merged
merged 1 commit into from
May 22, 2024

Conversation

zulus
Copy link
Contributor

@zulus zulus commented May 21, 2024

Simple fix for #1007

@rubenporras
Copy link
Contributor

thanks

@rubenporras rubenporras merged commit cf4c726 into eclipse:main May 22, 2024
7 checks passed
@rubenporras rubenporras self-requested a review May 22, 2024 06:31
@akurtakov
Copy link
Contributor

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

@rubenporras
Copy link
Contributor

Yes, no problem

@akurtakov
Copy link
Contributor

Let me push the lsp4j and guava updates first.

@rubenporras
Copy link
Contributor

rubenporras commented May 22, 2024

Thanks for pointing that out. I had plan to wait for #1010. That addresses LSP4J and Guava, or is any other PR coming?

@akurtakov
Copy link
Contributor

#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:

  • stay with old guava
  • use new guava with other deps from 2024-06 Orbit
  • use new guava from maven directly

@rubenporras
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Here you are #1011

akurtakov added a commit to akurtakov/wildwebdeveloper that referenced this pull request May 22, 2024
For critical fix for typescript ls
eclipse/lsp4e#1008
akurtakov added a commit to eclipse-wildwebdeveloper/wildwebdeveloper that referenced this pull request May 22, 2024
For critical fix for typescript ls
eclipse/lsp4e#1008
@zulus
Copy link
Contributor Author

zulus commented May 22, 2024

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

@zulus zulus deleted the PublishDiagnosticsCapabilities branch May 22, 2024 12:20
@mickaelistria
Copy link
Contributor

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
Copy link
Contributor Author

zulus commented May 22, 2024

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

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.

4 participants