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

Remove some of the deprecated for removal methods. #582

Merged

Conversation

rubenporras
Copy link
Contributor

No description provided.

@rubenporras
Copy link
Contributor Author

@mickaelistria , @BoykoAlex , @sebthom , as #556 but keeping LSPDocumentInfo. Would be this one good to be merged?

@rubenporras rubenporras force-pushed the removalKeepLSPDocumentInfo branch from b548043 to 7bbca93 Compare May 31, 2023 08:28
@rubenporras rubenporras force-pushed the removalKeepLSPDocumentInfo branch from 7bbca93 to 3f8c3dc Compare May 31, 2023 08:28
@rubenporras rubenporras merged commit 31bf765 into eclipse-lsp4e:master May 31, 2023
@rubenporras rubenporras deleted the removalKeepLSPDocumentInfo branch May 31, 2023 08:51
@vrubezhny
Copy link
Contributor

@rubenporras Well. this has killed some stuff in WildWebDeveloper. One (of several) case I cannot find a solution is here: https://github.com/eclipse/wildwebdeveloper/blob/master/org.eclipse.wildwebdeveloper.xml/src/org/eclipse/wildwebdeveloper/xml/internal/XmlLanguageClientImpl.java#L17

After this clean up PR merged, for WWD I cannot find an equivalent of removed public static CompletableFuture<Object> executeCommand(@Nullable Command command, @Nullable IDocument document, @Nullable String languageServerId)...

Problem is that the recommended replacement (as it stated in deprecation comments: @deprecated use {@link #executeCommandClientSide(Command, IDocument)) requires @NonNull IDocument document, while f.i. for @JsonRequest("xml/executeClientCommand") request there is no document nor resource specified in command arguments, so I cannot provide "@NonNull` document value.

@rubenporras
Copy link
Contributor Author

Hi @vrubezhny , looking at the removed code, execudeCommand just returned CompletableFuture.completedFuture(null) if document or command are null, so you should be able to do that in XmlLanguageClientImpl, don't you?

@vrubezhny
Copy link
Contributor

vrubezhny commented Jun 2, 2023

@rubenporras Not ready to tell right now what exact client commands can be executed, but in theory a command may not require a document - which was previously allowed.

Probably you're right, but then the deprecated call to CommandExecutor.executeCommand(cmd, null, null); looks like a stub-realization, never executing a real command...

In theory we could use CompletableFuture<Object> executeCommandClientSide(@NonNull Command command, @Nullable IPath path) with the default path ResourcesPlugin.getWorkspace().getRoot().getLocation() value for XmlLanguageClientImpl but it's not public.

@rubenporras
Copy link
Contributor Author

@vrubezhny , yes, you are right, we could make CompletableFuture<Object> executeCommandClientSide(@NonNull Command command, @Nullable IPath path) public, but the method we removed was doing nothing for your call and apparently no one detected that.

I wonder if it really the task of LSP4E to execute some command on the client side (without the server) and without a document. What is the use case? How is that related to LSP?

@mickaelistria
Copy link
Contributor

I wonder if it really the task of LSP4E to execute some command on the client side (without the server) and without a document. What is the use case? How is that related to LSP?

I think it can be some leftover of initial implementations when workspace/executeCommand operations wasn't clearly defined and there was room for interpretation about which of client or server should handle the command. LSP4E took the decision to use the client-side command execution as fail back. But as far as I know, this seems useless these days; and the only LS I'm aware of implementing such a pattern is LemMinX does it explicitly with a protocol extension.
Maybe the code to run a client-side command should just be moved to LemMinX?

@rubenporras
Copy link
Contributor Author

I think that would be good as it can be implemented unrelated to LSP. The code also does not particularly benefit from other code in LSP4E.

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.

3 participants