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

lsp: provide doc:show in hover, if available #1684

Merged
merged 1 commit into from
Apr 22, 2023

Conversation

sidkshatriya
Copy link
Contributor

@sidkshatriya sidkshatriya commented Mar 30, 2023

I am new to elvish. I am enjoying elvish very much, thank you!

The built in LSP feature in elvish is very useful but a bit basic at the moment.

One thing that I am running into while reading or writing elvish scripts in an editor (where I am connected to a elvish -lsp) is that I constantly need to refer to the documentation about a particular function (e.g. printf etc) in order to learn the finer aspects of the function.

This means I need to constantly do doc:show in the shell or refer to the elvish website and this breaks my editing "flow".

This PR automatically shows the doc:show (if available) for anything on the cursor in a hover.

The implementation is quite simple and it could be more sophisticated but it's working quite well for me.

@sidkshatriya sidkshatriya changed the title lsp: provide show:doc in hover, if available lsp: provide doc:show in hover, if available Mar 30, 2023
@sidkshatriya
Copy link
Contributor Author

Here is a sample screenshot of the hover feature in action:

hover_in_action

@sidkshatriya
Copy link
Contributor Author

@xiaq @krader1961 @zzamboni -- Would greatly appreciate a review and/or feedback on this feature... Thanks :-) !

@xiaq
Copy link
Member

xiaq commented Apr 3, 2023

Hi, thanks for the contribution. I usually review PRs in a FIFO manner, but here are some high-level feedback:

  • The MarkdownShowMaybe function seems unnecessary because you can just take the original Markdown source. The source function also already handles builtin commands.

  • The code to decide whether to look up doc is rather simplistic - for example, if you hover over the printf part of echo printf the doc of printf probably should not pop up.

Feel free to address those before I get back to this again, but this is a useful starting point and I wouldn't mind merging this as-is if you prefer.

@sidkshatriya
Copy link
Contributor Author

sidkshatriya commented Apr 4, 2023

Thanks !

The code to decide whether to look up doc is rather simplistic - for example, if you hover over the printf part of echo printf the doc of printf probably should not pop up.

Yes, this can definitely be improved.

Current algorithm

  1. Find a leaf node that has a range which includes the current cursor position
  2. Does the leaf node text have documentation? If so, simply return the documentation

This approach has some false positives like showing hover for echo printf that you pointed out.

Better Algorithm

  1. Find a leaf node that has a range which includes the current cursor position
  2. Once a leaf node is found, see if it is the correct type of node that could have documentation. We could use the parent property on a node to also traverse the tree upwards if this information lies at a higher level of the tree
  3. If the leaf node is found acceptable, see if there is any documentation. If so, return the documentation

I found the parsing/AST code a bit intimidating and was not sure how I should approach (2).

Can you provide some pointers for (2) ? Is there a pretty printer of some sort for the parse tree to get a feel of it ?

If you think it is too complicated to communicate tips for (2) then you could merge this commit as-is as you mentioned and improve upon this feature as you see fit.

@xiaq
Copy link
Member

xiaq commented Apr 22, 2023

Can you provide some pointers for (2) ? Is there a pretty printer of some sort for the parse tree to get a feel of it ?

There is already one place that does it, the completer for commands:

if np.match(expr, store(&form)) && form.Head == expr.compound {

The code is very concise thanks to some abstractions defined in node_path.go, but the basic idea is walking upwards from the leaf, and see if (1) we get a sequence of Primary, Indexing, Compound and Form nodes, and (2) the Compound node is equal to the Head field of the Form node.

@xiaq xiaq changed the base branch from master to lsp-hover April 22, 2023 19:53
@xiaq xiaq merged commit e7a591c into elves:lsp-hover Apr 22, 2023
xiaq added a commit that referenced this pull request Apr 22, 2023
As can be seen in the screenshot in
#1684 (comment), the on-hover
doc shows "```elvish" for the code block that contains the usage of the command,
which shouldn't appear in the rendered Markdown.

This is because the response uses the deprecated MarkedString format, where

> The pair of a language and a value is an equivalent to markdown:
> ```${language}
> ${value}
> ```

When this format is used, the "```elvish" code block is embedded inside another
"```markdown" codeblock, which leads to this weird rendering artifact.

Switching to the new MarkupContent format fixes this issue. However, the
github.com/sourcegraph/go-lsp is outdated and doesn't have this type defined, so
define it ourselves for now. In future we should switch to a more up-to-date Go
package that contains the LSP types.

Also remove the superfluous MarkdownShowMaybe function, and just export
doc.Source. The former did nothing useful other than prepending "# symbol". I
tested the on-hover docs of both Go and JavaScript in VS Code, both start with
the declaration of the symbol, not a heading.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants