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

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

Closed
hyangah opened this issue Oct 19, 2022 · 11 comments
Closed

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

hyangah opened this issue Oct 19, 2022 · 11 comments
Assignees
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@hyangah
Copy link
Contributor

hyangah commented Oct 19, 2022

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?

@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Oct 19, 2022
@gopherbot gopherbot added this to the Unreleased milestone Oct 19, 2022
@findleyr
Copy link
Contributor

Makes sense to me, and should be straightforward.

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

@adonovan
Copy link
Member

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

@danishprakash
Copy link

Is this something an external contributor can pick up?

@hyangah
Copy link
Contributor Author

hyangah commented Oct 21, 2022

@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 hyangah assigned danishprakash and unassigned hyangah Oct 21, 2022
@findleyr
Copy link
Contributor

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.

@dle8 dle8 added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Oct 27, 2022
@hyangah
Copy link
Contributor Author

hyangah commented Oct 27, 2022

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 hyangah added FeatureRequest and removed WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. labels Oct 27, 2022
@findleyr findleyr modified the milestones: gopls/v0.12.0, gopls/v0.13.0 Mar 27, 2023
@ShoshinNikita
Copy link

  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()
    ```
    

@findleyr
Copy link
Contributor

findleyr commented Jun 14, 2023

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
Copy link

  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.

@findleyr findleyr modified the milestones: gopls/v0.14.0, gopls/v0.15.0 Oct 11, 2023
@gopherbot
Copy link

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

@findleyr findleyr assigned adonovan and unassigned danishprakash Jan 17, 2024
gopherbot pushed a commit to golang/tools that referenced this issue Jan 19, 2024
This change updates the logic to display the method set of the
selected type so that:
- it includes promoted methods;
- it includes interface methods, if that would not be
  redundant with the syntax of the the type;
- receiver variables are properly named, and have a
  pointer type if appropriate.

Fixes golang/go#61634
Updates golang/go#56331

Change-Id: Ied9c06c36178424575012adb8fde0b1ddbd688c3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/555456
Reviewed-by: Robert Findley <rfindley@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
@gopherbot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

7 participants