wrap command return type #452
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tic messages can carry a list of quickfixes. Only for DSLs though
…toring the CompletableFuture/Stream code for code actions
CodeActions. Commands were the simplest way of creating this interaction, but the LSP provides more configurability and smoother UI interactions via the concept of a "CodeAction". A CodeAction optionally wraps a Command and/or a DocumentEdit. The edit is always executed first and may involve preview(s) of the different alternative commands. The command is just the command to execute when the option is chosen, just as before. Command titles are lifted to CodeAction titles if none are present. CodeActions may be "categorized" via the CodeActionKind enum. This enum is open for users to extend. The current definition lists the currently known list in lsp4j. If a command calls a IDEServices.registerTextEdits, then this has the same effect on the files and editor content as providing the edits in the CodeAction immediately. However, UI interactions like previews and OK/Cancel pop-ups may be different.
…t on Mac at least you can not multitask while the tests are running
…asynchronous command execution
… the LSP server for robustness and type-safety
|
Also after this PR, if we don't replace |
|
So this seems like a small change, but it breaks the API of every command executor. I do think it is necessary for the future of util::LanguageServer to clean this up. |
|
There is a big TODO in this code that would break the client code on the TypeScript side even more. So let's push that into the future. It is also not strictly necessary. |
|
I think something went wrong with this PR, it has a lot of commits in here? I think from the code-actions branch? |
|
Yes I branched off the wrong branch. Let's wait and merge the other one to
main first; then I'll rebase these changes.
Jurgen Vinju
http://jurgen.vinju.org
CWI SWAT
TU Eindhoven
Swat.engineering BV
…On Thu, 19 Sep 2024 at 15:10, Davy Landman ***@***.***> wrote:
I think something went wrong with this PR, it has a lot of commits in
here? I think from the code-actions branch?
—
Reply to this email directly, view it on GitHub
<#452 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAPF5FY7F7M6AS4X4ZFJU6LZXLELNAVCNFSM6AAAAABOP3AOVCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNRQHE2DKMZUGI>
.
You are receiving this because you were assigned.Message ID:
***@***.***>
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current CommandExecutor definition is
value (Command _command), so it can return anythingincluding
trueand1.However, if we do that the JSON-RPC bridge breaks. The LSP4J method to implement is:
public CompletableFuture<Object> executeCommand(ExecuteCommandParams params) {But
Objectmust not be primitive or become primitive.Therefore the current code in Rascal language servers (repeated for every language times every command) wraps every result in a map like so:
return ("result" : true)This PR changes that such that we can write:
return true;And for backward compatibility it wraps the result before sending it over the bridge:
return vf.map().put(vf.string("result"), result);This way the Rascal type-checker can completely check the validity of the CommandExecutor again,
and the JSON-RPC bridge can not accidentally fail anymore.
If there exists code that wraps CommandExecutor results differently, e.g. like so:
return [result];then this PR breaks those language servers. Therefore it would be good for @DavyLandman to have a close
look at this before we merge it.