-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Filter out code actions if unsupported by the client and advertise our capabilities #4167
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
Conversation
crates/rust-analyzer/src/caps.rs
Outdated
code_action_provider: Some(CodeActionProviderCapability::Simple(true)), | ||
code_action_provider: Some(CodeActionProviderCapability::Options(CodeActionOptions { | ||
code_action_kinds: Some(vec![ | ||
"".to_string(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this probably needs comment (and could be String::new()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will make a PR to lsp-types to put in Empty
// If the client only supports commands then filter the list | ||
// and remove and actions that depend on edits. | ||
if !world.config.client_caps.code_action_literals { | ||
res = res |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think .retain
would be a more convenietn API to use here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah no, we do need to transform things... https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.drain_filter would work, but it's nighly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment to move to drain_filter
once stable.
Ok(CodeAction { | ||
title, | ||
kind, | ||
kind: Some("refactor".to_owned()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like using Empty
by default, unless the kind is set explicitelly, is better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defaulting to Empty
makes sense. I just wanted things to show up in the Refactor menu but was too lazy to go through the work of restructuring everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now default to Some(String::new)
which I think is fine until we start assigning kinds.
Even thought we don't return all of these we eventually will so might as well advertise now.
Most of them area. We will separate them out later but this gets them to show up in the "refactor" menu of vscode.
bors r+ |
Build succeeded: |
This PR does three things:
CodeActionKind
this will filter the results and only sendCommand[]
back.CodeActionKind
. This may cause clients to not request code actions if they are checking for the provider to betrue
(or implement LSP < 3.8) in the caps but I will fix that in a followup PR."refactor" so that they show up in the menu in vscode.""
.Part of #144
#4147
#2833