x/tools/gopls: consider the methods list at the end of the hover

[hyangah]

gopls version v0.10.0-pre.1
go version 1.19.1

It looks like gopls places
the type definition
list of methods (exported, and unexported)
type comments

When the list of methods is long, it needs quite some scrolling to get to the documentation. (example gopls/internal/lsp/cache/workspace.go, hover over workspace in line 160.

How about placing the method list at the end?

[findleyr]

Makes sense to me, and should be straightforward.

This feature was added in https://go.dev/cl/420714.

[adonovan]

Thanks for the feature request. I have assigned it to one of our experts.

[danishprakash]

Is this something an external contributor can pick up?

[hyangah]

@danishprakash Sure! Assigning this to you.

One question I have is whether we should do this conditionally or not.

In v0.10.0, the hover message over a struct type will present in the order of:

• type spec
  • shape of the struct
  • exported methods
  • unexported methods
• doc comment

When the number of methods are small, I think this layout works well and consistent.
Especially, when the doc comment is rather long, I appreciate the current order for me to quickly take a look at the available methods.

When the number of methods are long and the doc comment is short, placing the doc first sounds more convincing.

[findleyr]

One question I have is whether we should do this conditionally or not.

Do you mean via configuration, or dynamically via some heuristic for the optimal layout.

I think we should be consistent with pkgsite, which would put the type definition first, followed by doc, followed by methods. Unlike pkgsite we shouldn't include documentation for the methods, as that would be too verbose.

[hyangah]

I think we should be consistent with pkgsite,
Agree. Thanks for finding a precedent case.

• Type definition
• Doc
• List of methods - note: unlike pkgsite, this can include unexported methods.

[ShoshinNikita]

1. Where should we place the link to pkg.go.dev? Right now it is at the end of the hover. I think we can leave it at the end of the docs, just before the methods list. But I always hide this link - so, it would be great to hear others' thoughts.

   Now	After the docs	Before the docs

2. Can we modify gopls/internal/lsp/source.HoverJSON (as I understand, it doesn't follow any specific format)? It looks like the easiest way to fix this issue is to create a new field that will hold "extra" information. Something like this:

   type HoverJSON struct {
     ...
   
     // Extra holds additional information - for example, methods list.
     Extra string `json:"extra"`
   }

3. I noticed that VSCode ignores all the newlines between the code blocks and doesn't add a gap (the number of \n doesn't matter). It is not a huge problem, but it doesn't look right to me. As a workaround we can use <br>, but I am not sure we should place HTML tags inside markdwon (at least in this case).

   Now	New Lines	<br>
   ```go type X struct{} func (X).A() func (X).B() func (X).C() ```	```go type X struct{} ``` ```go func (X).A() func (X).B() func (X).C() ```	```go type X struct{} ``` <br> ```go func (X).A() func (X).B() func (X).C() ```

[findleyr]

Thanks for the questions, @ShoshinNikita. Quick thoughts below.

1. I also don't know where the best location for the link is, and want to hear from others.
2. Yes, we can modify HoverJSON. JSON format is only used by govim, afaik, if at all, and adding a field is safe.
3. A <br> might not be valid across different LSP clients. Perhaps an empty code block..?

[ShoshinNikita]

3. It looks like VSCode either renders code blocks or ignores them. However I managed to find another possible solution - just add 2 extra \n at the end of a signature. The only issue is that it doesn't work well when docs are presented:

   No docs	Docs
   ```go type X struct{} ``` ```go func (X).A() func (X).B() func (X).C() ```	```go type X struct{} ``` X ... ```go func (X).A() func (X).B() func (X).C() ```

   Therefore, this solution would require additional logic for cases when either docs or the link are not empty. Also it can potentially break rendering in other IDEs. Another option is to just leave it as is and create an issue in the VSCode repo.