go icon indicating copy to clipboard operation
go copied to clipboard

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

Open hyangah opened this issue 3 years ago • 6 comments

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?

hyangah avatar Oct 19 '22 19:10 hyangah

Makes sense to me, and should be straightforward.

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

findleyr avatar Oct 19 '22 21:10 findleyr

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

adonovan avatar Oct 20 '22 16:10 adonovan

Is this something an external contributor can pick up?

danishprakash avatar Oct 21 '22 05:10 danishprakash

@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.

hyangah avatar Oct 21 '22 14:10 hyangah

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.

findleyr avatar Oct 21 '22 14:10 findleyr

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.

hyangah avatar Oct 27 '22 17:10 hyangah

  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
    now after docs before 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>
    now gap new line 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()
    ```
    

ShoshinNikita avatar Jun 14 '23 21:06 ShoshinNikita

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..?

findleyr avatar Jun 14 '23 21:06 findleyr

  1. 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.

ShoshinNikita avatar Jun 14 '23 22:06 ShoshinNikita

Change https://go.dev/cl/555456 mentions this issue: gopls/internal/lsp/source: show promoted methods in hover

gopherbot avatar Jan 12 '24 18:01 gopherbot

Change https://go.dev/cl/559495 mentions this issue: gopls/internal/golang: hover: show type's methods after doc comment

gopherbot avatar Jan 30 '24 15:01 gopherbot