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.

Sign up for GitHub

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

Jump to bottom

Use LSP4J 0.20.0 #2417

Merged
merged 2 commits into from
Feb 23, 2023
Merged

Use LSP4J 0.20.0 #2417

merged 2 commits into from
Feb 23, 2023

Conversation

mickaelistria
Copy link
Contributor

  • Bump versions & adopt new APIs (eg remap globPatterns to String)
  • Remove custom text hierarchy commands; use LSP standards
  • Remove custom inlayHints, use standard
  • Declare null notebook service

@mickaelistria mickaelistria mentioned this pull request Jan 24, 2023
Closed
@mickaelistria mickaelistria changed the title Make it work with newer LSP4E Use LSP4J 0.19.0 Jan 24, 2023
@rgrunber
Copy link
Contributor

We can proceed with the migration. @CsCherrYY Would be good to confirm this handles everything in #2033 .

@rgrunber rgrunber added this to the End February 2023 milestone Jan 26, 2023
@rgrunber rgrunber added dependencies Pull requests that update a dependency file enhancement labels Jan 26, 2023
@mickaelistria
Copy link
Contributor Author

@CsCherrYY If this helps, I can try to remove from that patch the parts about TypeHierarchy. Feel free to request it if needed.

@CsCherrYY
Copy link
Contributor

as for type hierarchy part, if we decide to move the implementation to LSP, we should also replace the client implementation: redhat-developer/vscode-java#2376 and it's precondition: redhat-developer/vscode-java#2377

@rgrunber I will rebase redhat-developer/vscode-java#2377 recently so that you can proceed the review process :)
@mickaelistria I'm OK to remove the typehierarchy part from this PR and leave them in #2033, since there are several things not covered by the protocol and should be implemented in the java language server independently (e.g., class hierarchy)

@mickaelistria
Copy link
Contributor Author

I have removed the migration to newer Typehierarchy from this PR.

@snjeza
Copy link
Contributor

snjeza commented Jan 30, 2023

@mickaelistria should we update gson to 2.10.1?

@mickaelistria
Copy link
Contributor Author

should we update gson to 2.10.1?

In this patch, I've removed direct reference to gson version so it just resolves the right one transitively from LSP4J requirements. So I don't think more changes are necessary.

@mickaelistria mickaelistria force-pushed the lsp4j-0.19.0 branch 2 times, most recently from 95980c7 to 1b0473c Compare February 1, 2023 20:37
mickaelistria added a commit to redhat-developer/eclipseide-jdtls that referenced this pull request Feb 1, 2023
@mickaelistria
Improve support for in-process LS
208070e 
* use flag proposed in
eclipse-jdtls/eclipse.jdt.ls#2423 to prevent undesired
"echo" of changes, markers, logs
* Added requirements to m2e and buildship to workaround
eclipse-jdtls/eclipse.jdt.ls#2429
* Updated TP to newer versions
* Updated Tycho version
* Currently added jdt-ls in same build as submodules to be able to used
the "good" branch, compatible with Eclipse IDE, ie including
  ** eclipse-jdtls/eclipse.jdt.ls#2423 and
  ** eclipse-jdtls/eclipse.jdt.ls#2417
@mickaelistria
Copy link
Contributor Author

I don't think the failure is related. Can someone check this and confirm/infirm my impression?
What else is left to do before this can be merged?

@datho7561
Copy link
Contributor

I ran the tests on the master branch locally, and they passed.

@snjeza
Copy link
Contributor

snjeza commented Feb 6, 2023

test this please

@datho7561
Copy link
Contributor

I ran mvn clean verify for this PR locally and it passed

@mickaelistria
Onboard legacy TypeHierarchy API
c9e7d82 
LSP/LSP4J doesn't maintain those API anymore, so let's include them into
JDT-LS source code to allow moving JDT-LS to newer LSP/LSP4J version
while still keeping current typeHierarchy commands.
@mickaelistria mickaelistria changed the title Use LSP4J 0.19.0 Use LSP4J 0.20.0 Feb 21, 2023
rgrunber
rgrunber approved these changes Feb 21, 2023
View reviewed changes
Copy link
Contributor

@rgrunber rgrunber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good to me. Tried it out with redhat-developer/vscode-java#2377 and didn't find any problems. Once the issues there get resolved, I think we can merge this (although I tried, and this even seemed to work with the older client version).

org.eclipse.jdt.ls.core/src/org/eclipse/jdt/ls/core/internal/JavaClientConnection.java
@@ -76,12 +74,6 @@ public interface JavaLanguageClient extends LanguageClient, ExecuteCommandPropos
@JsonNotification("language/progressReport")
void sendProgressReport(ProgressReport report);

// TODO : remove this method when LSP4J will provide InlayHint support. See
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just remove the same comment at https://github.com/eclipse/eclipse.jdt.ls/pull/2417/files#diff-28f5ba61a1d80eab1f3636300e9f9719268850930cb2609f7833d9502d5e4a4bR216-R217 of this file. That method gets to stay as it's a helper.

@rgrunber
Copy link
Contributor

re-test this please.

@mickaelistria
Use LSP4J 0.20.0
9d93838 
* Bump versions & adopt new APIs (eg remap globPatterns to String)
* Remove custom inlayHints, use standard
* Declare null notebook service
@rgrunber rgrunber merged commit c13e9ca into eclipse-jdtls:master Feb 23, 2023
@rgrunber rgrunber linked an issue Feb 23, 2023 that may be closed by this pull request
Update to newer LSP4J #2348
Closed
@mickaelistria
Copy link
Contributor Author

Thanks!

@rgrunber rgrunber removed this from the Mid March 2023 milestone Mar 15, 2023
@rgrunber rgrunber mentioned this pull request Jun 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrunber rgrunber rgrunber approved these changes

Assignees
No one assigned
Labels
dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update to newer LSP4J
5 participants
@mickaelistria @rgrunber @CsCherrYY @snjeza @datho7561