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

Move TypeHierarchy to LSP #2033

Closed

Conversation

CsCherrYY
Copy link
Contributor

@CsCherrYY CsCherrYY commented Mar 24, 2022

Related to redhat-developer/vscode-java#2376, more details can be found in that PR.

Signed-off-by: Shi Chen [email protected]

@CsCherrYY CsCherrYY force-pushed the cs-typehierarchy-lsp branch from abab74a to 40f66f6 Compare April 18, 2022 02:45
@rgrunber rgrunber mentioned this pull request Jan 24, 2023
@CsCherrYY CsCherrYY force-pushed the cs-typehierarchy-lsp branch 4 times, most recently from edfab69 to 651942d Compare March 27, 2023 07:53
@CsCherrYY CsCherrYY marked this pull request as ready for review March 27, 2023 08:04
@mickaelistria
Copy link
Contributor

Note that we have a strong interest in this feature in eclipseide-jdtls.

@CsCherrYY CsCherrYY force-pushed the cs-typehierarchy-lsp branch from 651942d to c36d4bb Compare March 30, 2023 01:31
@mickaelistria
Copy link
Contributor

Hi,
any chance we can get it merged soon?

@rgrunber
Copy link
Contributor

rgrunber commented Jun 28, 2023

As far as I understand the type hierarchy API in the LSP spec supports supertypes & subtypes but not both at once (class hierarchy). Looks like this was mentioned at redhat-developer/vscode-java#2376 (comment) (under cons). So we would still have to have a separate delegate command / protocol extension that does the class hierarchy. I don't think that would be hard though.

@jdneo would it be a pain to adopt the new LSP spec and just change the LSP methods called on the client (VS Code) side ? So instead of calling the delegate methods, we just call the new (LSP) ones ? I don't think I understand fully, but I assume we cannot rely on the VS Code UI + the language client to fully handle the type hierarchy UI because we like our "class hierarchy by default" approach. But it would at least mean all clients can use the new API. After that, it's up to each client to figure out what they want their default behaviour to be.

@mickaelistria
Copy link
Contributor

Note that from other clients (eg eclipseide-jdtls) perspective, we don't mind if another command exists and vscode-java uses it; what's frustrating is just to not have the standard LSP operations supported, although a patch provides them. Keeping both would be great and would allow vscode-java to keep the legacy command for some time, until it can replace it by standard LSP operations, but I see this patch as a pre-requisite before such a change can happen in vscode-java; and definitely as a (not-delivered-yet) feature for high value for all other clients.

@mickaelistria
Copy link
Contributor

Can we merge it without waiting for vscode-java to migrate? If necessary, I can add some extra preferences to make the standard type hierarchy opt-in to prevent conflicts with legacy one is VSCode (if there is any conflict?)

@rgrunber
Copy link
Contributor

Ok, so let's just keep the old delegate commands and also provide the new endpoints supported by the LSP spec. Ideally if the new endpoints can just take advantage of the existing delegate methods, which I suspect the current PR already does.

This way at least clients like vscode-java are given some grace period in terms of migration.

@mickaelistria
Copy link
Contributor

I've opened #2769 where I started from this commit but amended it so the existing typehierarchy command remain and can still be used by VSCode as long as they are necessary.

@rgrunber
Copy link
Contributor

rgrunber commented Sep 5, 2023

Closing as I think Mickael took the lsp-spec part of the code here and simply opted to leave the delegate part in-place at #2769. Feel free to re-open If I've missed something.

@rgrunber rgrunber closed this Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants