-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Hover actions #4729
Hover actions #4729
Conversation
Since this is a protocol extension we might want a heavy test for when this is and isn't supported. |
@kjeremy thanks! I wasn't aware of |
30d516a
to
4193ad7
Compare
impl Default for HoverConfig { | ||
fn default() -> Self { | ||
Self { implementations: true } | ||
} | ||
} |
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.
Since the client has to opt in to this feature it makes more sense for the default to be false.
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.
Initially it was.
matklad said(#4659 (comment))
I think in ra_ide we generally enable everything by deafault, and than rust-analyzer explicitelly disables stuff it doesn't like.
pub fn to_markup(&self) -> String { | ||
self.results.join("\n\n---\n") | ||
self.results.join("\n\n___\n") |
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.
Is this correct? We just went back and forth on this a few PRs ago.
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 believe yes, it is. As I can see '---' was replaced by '___', but not in this place.
---
cause problems if hover content is just a single line.
some test
---
Becomes
some test
while
some test
___
some test
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.
SGTM
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.
Btw, hover actions are rendered on the client and this method is not used for them at all.
You'll need to rebase |
4193ad7
to
a0dfb72
Compare
Rebased. |
"range": { "end": { "character": 13, "line": 2 }, "start": { "character": 7, "line": 2 } } | ||
}) | ||
); | ||
} |
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'd rather not add heavy tests for this functionality:
- the cost of a heavy test is high, as it spawns an external process
- the additional confidence it provides is low, as it only verifies the JSON we produce, and not how the other side interprets the JSON.
So, almost all code in handlers.rs
and conv.rs
is deliberately not tested, and we just rely on types and manual testing for correctness, and on keeping these modules trivial-ish.
So far, it worked ok -- the regressions I remember usually involve bugs in https://github.com/Microsoft/vscode-languageserver-node/
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'm not against the deleting. Just one consideration:
additional confidence it provides is low, as it only verifies the JSON we produce,
I do not think so. Such tests let us to ensure that we produce valid JSON.
For example, these tests helped me to find and fix a problem: with disabled hoverActions client capability, the server was still adding empty actions field to the Hover response. Of course, LSP defines that an unknown property should be ignored, but it is not always the case.
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 still think it makes sense to delete. I agree that this can help to find some bugs, probably quite a few of them. But, over the lifetime of the project, the cost of running&maintaining these test seems like it would be higher than the additional benefit.
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.
(as a more general design note, I find that for ironing bugs on the integration boundary the most efficient strategy is release trains (so that folks can try things out in various real world configurations) and speedy release process (so that you can submit hotfix and make a new release quckly))
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.
yea, this strategy should work well for such an active project :)
| TITLE _Action1_ | _Action2_ | <- second group | ||
+-----------------------------+ | ||
... | ||
``` |
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.
Hm, I feel like this suffers the same problem as codeLens request -- it necessitates client-specific code. I guess it's not something we can fix on our side.
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.
The client has to explicitly set the capability field and to implement some commands anyway (rust-analyzer.showReferences
in this case) therefore I think it is ok.
Besides, I hope that it would be possible to support hover actions functionality in editors.
I've checked: in vscode it is very easy to add, in sublime it already exists and one just need to get information from the new Hover response :)
editors/code/src/client.ts
Outdated
return md; | ||
} | ||
return obj; | ||
} |
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.
Is this really trusted? Should we do some kind of validation/escaping 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.
Good catch!
Indeed someone can write in a doc comment something like
[title]('command:some.vscode.command')
and we will render the corresponding link in the hover without any validation. I left this code in the hope it would be useful to implement intra-doc links later. But now I think I'd better remove it, and if we will support intra-doc links we have to add some validation. But not in this PR.
Command links added to the hover bottom are safe as we fully control them. So this code is safe in turn: hover.contents.push(renderHoverActions(actions));
/// Contains the results when hovering over an item | ||
#[derive(Debug, Default)] | ||
pub struct HoverResult { | ||
results: Vec<String>, | ||
actions: Vec<HoverAction>, | ||
} |
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.
Yup, this API makes sense to me!
LGMT, with testing strategy changed to not rely on heavy tests. |
Hover contents might be extracted from raw doc comments and need some validation.
a0dfb72
to
bd9d7b6
Compare
It seems to be OK now. |
bors r+ Thanks @vsrs ! |
Summary: The change introduces support _hover actions_, a LSP extension modeled after the Rust Analyzer one (introduced [here](rust-lang/rust-analyzer#4729)). Hover actions are appended, on the client side, as _command links_ to the bottom of a hover popup. They are disabled by default for now. The LSP extension works by extending the standard `Hover` result with optional actions. Change Highlights: * Introduce two configuration values: ` hoverActions_enable` (to control hover actions as a whole) and `hoverActions_docLinks_enable` (to control hover actions of type `docLink`) * Introduce the LSP extension * Introduce a `DocLink` action to point to external docs, when available Reviewed By: alanz Differential Revision: D48154706 fbshipit-source-id: 45a1850a03e350d44cad046f4222f66a1f981d56
This PR adds a
![hover_actions_impl](https://user-images.githubusercontent.com/62505555/83335732-6d9de280-a2b7-11ea-8cc3-75253d062fe0.gif)
hoverActions
LSP extension and aGo to Implementations
action as an example: