Skip to content

client/executeCommand #1119

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

Open
wants to merge 2 commits into
base: gh-pages
Choose a base branch
from
Open

client/executeCommand #1119

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a2cea3e
client/executeCommand
kdvolder Oct 29, 2020
baa4d4f
Be more specific about error response
kdvolder Oct 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion _specifications/specification-3-16.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2715,7 +2715,7 @@ _Response_:
* partial result: `SymbolInformation[]` as defined above.
* error: code and message set in case an exception happens during the workspace symbol request.

#### <a href="#workspace_executeCommand" name="workspace_executeCommand" class="anchor">Execute a command (:leftwards_arrow_with_hook:)</a>
#### <a href="#workspace_executeCommand" name="workspace_executeCommand" class="anchor">Execute a server command (:leftwards_arrow_with_hook:)</a>

The `workspace/executeCommand` request is sent from the client to the server to trigger command execution on the server. In most cases the server creates a `WorkspaceEdit` structure and applies the changes to the workspace using the request `workspace/applyEdit` which is sent from the server to the client.

Expand Down Expand Up @@ -2779,6 +2779,36 @@ _Response_:
* result: `any` \| `null`
* error: code and message set in case an exception happens during the request.

#### <a href="#client_executeCommand" name="client_executeCommand" class="anchor">Execute a client command(:leftwards_arrow_with_hook:)</a>

The `client/executeCommand` request is similar to `workspace/executeCommand` but is sent from the server to the client, to trigger command execution on the client.

_Client Capability_:
* property path (optional): `client.executeCommand`
* property type: `ClientExecuteCommandClientCapabilities` defined as follows:

```typescript
export interface ClientExecuteCommandClientCapabilities {
/**
* The client supports message 'client/executeCommand'.
*/
supported: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

I get that you're trying get something out the door but this needs many more options for clients. Given just this state of the PR a server doesn't know whether a command can succeed or not.

Just to give you some ideas:

  • the client should be able to communicate dynamically to the server about possible new commands that it knows about, similar to workspace/didChangeConfiguration.
  • It should be possible for the server to get a list of known command names from the client, similar to workspace/configuration

At this point the server can only do client/executeCommand and hope for the best.

Copy link

Choose a reason for hiding this comment

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

I don't think we should require more options at this point. The current proposal is good and there is room for evolution (such as listing commands) in the future. However, we don't really have a strong need to pass a static list of commands at that point, just like symmetrically, there is no strong need for clients to know in advance the list of server commands it can invoke.
The command pattern is often use for client or LS specific integrations, that we cannot really standardize in LSP because they're not generic enough.
I think it's better to mimic the current workspace/executeCommand action (and related capabilities), it seems more consistent.

At this point the server can only do client/executeCommand and hope for the best.

It's the same for workspace/executeCommand, and yet this is working well ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

It's the same for workspace/executeCommand, and yet this is working well ;)

Not quite. If you're lucky, you can read a readme file explaining what the arguments of a command can be. If you're unlucky, you have to dig through the server source code to figure out what a command can do.

Copy link
Contributor

Choose a reason for hiding this comment

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

The configure-less way of invoking commands is via code actions, which works well, I agree.

Copy link
Author

@kdvolder kdvolder Oct 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not against adding more options in the future for things such as 'command discovery' or making the spec more precise about mechanism by which commands are made to 'exist' on the client side.

However, I wanted to explicitly avoid this becoming yet another ticket dragged out into a lengthy discussion with no actionable outcome.

To me these questions are somewhat orthogonal:

  1. how does a server request a client to execute a command
  2. how does a server know that a client can execute a specific (set of) commands
  3. how are client-side commands defined

So I explicitly wanted limit the scope of this ticket to be just about 1. The other questions can be answered independently and in fact there are already some tickets open with long discussions.

Given just this state of the PR a server doesn't know whether a command can succeed or not.

That is true, but at least we know how to request execution in a standardised way. I.e. we are answering question 1. So to me that is progress.

And we can assume an error comes back if the execution failed (I agree with your other comments that we can perhaps make some of the error cases more explicit in the spec).

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries. I am just some guy maintaining a client and weary of opening pandora's box with client/executeCommand. We must have a follow-up PR that defines how command discovery works, and we must standardize some "common" commands.

Copy link
Author

Choose a reason for hiding this comment

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

I don't really disagree with you. I just don't want to go there yet. Yes, I could imagine some options added to capability object in the future such as 'commands: [...]' for a list of 'statically' known commands. Or a property 'commandDisovery: boolean' that indicates there is some extra protocol for command discovery. etc. But this sort of thing could be arbitrarily complex and with lots of people wanting something a little different from it. So I just don't think we should/can tackle all of these at once right now. The more we can separate this into smaller questions/issues, the more chance we have of actually making progress in small increments.

}
```

_Request:_
* method: 'client/executeCommand'
* params: `ExecuteCommandParams` defined exactly the same way as for 'workspace/executeCommand'.

_Response_:
* result: `any` \| `null`
* error: code and message set in case an exception happens during the request.

The client shall return `ErrorCode.InvalidParams` when the server requests

- a command that doesn't exist
- invalid params for the command signature

#### <a href="#workspace_applyEdit" name="workspace_applyEdit" class="anchor">Applies a WorkspaceEdit (:arrow_right_hook:)</a>

The `workspace/applyEdit` request is sent from the server to the client to modify resource on the client side.
Expand Down