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

Add CodeAction class #389

Closed
mjbvz opened this issue Jan 25, 2018 · 18 comments
Closed

Add CodeAction class #389

mjbvz opened this issue Jan 25, 2018 · 18 comments
Labels
help wanted Issues identified as good community contribution opportunities
Milestone

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Jan 25, 2018

VS Code 1.20 adds a new CodeAction type that captures additional information about codeactions:

This new type is used by several new product features such as the Refactor command and the editor.action.codeAction command. It should also be supported in the LSP for code action provides

@yaohaizh
Copy link

yaohaizh commented Mar 9, 2018

Second this. @aeschli

Current, the lsp CodeAction return a list of Command. It is hard to differentiate these commands. In test cases, we need do some filter based on the commands.

@gorkem
Copy link
Contributor

gorkem commented Mar 12, 2018

+1
I think the WorkspaceEdit field is also an improvement on the references object. Most code actions are just workspace edits and some LSPs are doing a round trip for no good reason other than specification's short comings.

@mickaelistria
Copy link

Seems very similar to proposal in #178

@dbaeumer dbaeumer added this to the On Deck milestone Apr 9, 2018
@dbaeumer dbaeumer added the help wanted Issues identified as good community contribution opportunities label Apr 9, 2018
@dbaeumer
Copy link
Member

dbaeumer commented Apr 9, 2018

See also the discussion in #432. We should work on this sooner than later since it will make things a lot easier to understand when it comes to commands and code actions.

@rcjsuen
Copy link
Contributor

rcjsuen commented Apr 13, 2018

New OrganizeImports CodeActionKind being considered, see microsoft/vscode#47845.

@mjbvz
Copy link
Contributor Author

mjbvz commented Apr 18, 2018

I just merged in the OrganizeImports addition from microsoft/vscode#47850. From the API perspective, all it did was add a new constant.

I'd like to get other extensions that have their own organize imports (such as java) to try to migrate over to using a code action instead. This is blocked until we have basic support for a CodeAction class in the LSP

@mickaelistria
Copy link

I would like to work on this, however I've got some preliminary questions:
Does the proposal for the spec have to follow the current class in VSCode? I'm not convinced we need to specify the codeActionKind from 1st round, and I also find more correct for the protocol to clearly use Command|WorkspaceEdit explicitly instead of having 2 distinct field and a comment saying they're exclusive.
Also, should the protocol add some client/server capability to declare what kind of codeActions results are expected/supported (current command-only as default value, codeAction class as an option)? Or can we just move it forward and let clients implement some tricks for backward compatibility (LSP4J is good at it as it can have the request return an Either<Command[]|CodeAction[]> for the codeAction request, which is IMO simpler to use than capabilities to decide of how to handle the response).

@mjbvz
Copy link
Contributor Author

mjbvz commented May 2, 2018

The kind is a big reason we added the CodeAction class.VS Code uses it split code actions by type in order to create different menus for refactoring actions and source action, and I imagine other tools will want to do something similar.

The command and edit actually aren't implemented as exclusive. We apply the command and then the command. I'll update the vscode docs to make this clear.

One other point: we also added an only field to CodeActionContext. This is used to say which types of code actions a code action provider should return.

@dbaeumer dbaeumer modified the milestones: On Deck, May 2018 May 8, 2018
@dbaeumer
Copy link
Member

dbaeumer commented May 8, 2018

Actually this got added to the VS Code May plan :-).

@mickaelistria I still appreciating your help here. I will have a look beginning of next week and then add a proposal how I think we can best implement this. Knowing that the Java implementation solved this with an Either type I am actually leaning towards having this as or types in the spec.

@dbaeumer
Copy link
Member

Here is a PR for a CodeAction type in the protocol: microsoft/vscode-languageserver-node#350

Clients need to opt into allowing textDocument/codeAction requests returning CodeAction literals. In addition the kind value set is announced by the client as well.

Feedback welcome.

@yaohaizh
Copy link

yaohaizh commented Jun 8, 2018

@dbaeumer microsoft/vscode-languageserver-node#350 is merged. When will the vscode language server protocol spec will be updated?

@dbaeumer
Copy link
Member

@yaohaizh sorry. Forgot about it in the endgame stress. Will do today.

@mickaelistria
Copy link

Thank you very much!

@dbaeumer
Copy link
Member

You are welcome.

@Gama11
Copy link
Contributor

Gama11 commented Jul 25, 2018

I was having some trouble getting the "Organize Imports" command and "Source Action..." context menu item to show up in VSCode after implementing CodeActionKind.SourceOrganizeImports in the Haxe Language Server (despite it working fine with "editorCodeActionsOnSave"). After some digging, I stumbled over this in the 1.23 release notes:

https://code.visualstudio.com/updates/v1_23#_codeactionprovidermetadata

So if I understand correctly, a language server currently can't have proper "Organize Imports" support in VSCode because CodeActionProviderMetadata is missing from the protocol? The same would apply to the Refactor... menu. Or am I missing something?

@mjbvz
Copy link
Contributor Author

mjbvz commented Jul 28, 2018

@Gama11 Was this the same problem you were seeing with microsoft/vscode#54803 ?

@Gama11
Copy link
Contributor

Gama11 commented Jul 28, 2018

No, both have to do with Code Action kinds, but this is unrelated.

Basically, the Language Server Protocol is missing a way to for the server specifiy what kinds of code actions will be provided (CodeActionProviderMetadata), meaning you can't get these context menu items (and the "Organize Imports" command) to show up in VSCode:

This is documented in the 1,23 release notes which I linked to earlier. I would have expected this to be part of ServerCapabilities, but codeActionProvider is still just a simple boolean right now. As a result, the registerCodeActionProvider() call in vscode-languageclient is not providing this metadata.

@vscodebot vscodebot bot locked and limited conversation to collaborators Jul 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Issues identified as good community contribution opportunities
Projects
None yet
Development

No branches or pull requests

7 participants