Is your feature request related to a problem? Please describe. Package-level description isn't rendering
Describe the solution you'd like I'd like the package-level description to render when hovering over an imported package, as shown in the screenshots below. ChatGPT suggests that if a package is spread across multiple files, Go uses first (alphabetically) nonempty comment, but I'm not totally sure. I don't know if VSCode is just getting confused.
Describe alternatives you've considered One option is to ignore it. Another is to use GoLand. Another is to just always have the docs up
Additional context
GoLand
I know the docs are there. They're in doc.go, and they're rendering in Godoc, and they're showing in GoLand
https://pkg.go.dev/go.temporal.io/sdk@v1.31.0/workflow#Go
VSCode
Comment From: GSmithApps
I might be able to help out on this too
Here's the Godoc github. I figure their logic is in here somewhere. Maybe we could use that
Comment From: firelizzard18
@GSmithApps There's already logic in gopls for rendering package documentation. If you set "gopls": { "ui.documentation.hoverKind": "FullDocumentation" } and hover over the import URL it will render the full package documentation (the image below). The hover implementation is here. I'm not sure precisely what needs to be changed, but I believe it would only require a minor modification to hover.go. But this might not be something most users want to see. CC @findleyr
Comment From: GSmithApps
Hey @firelizzard18 thanks so much for looking at this! Yeah I'm having luck when I hover over the import statement, but not when I hover over the variable name that it imports to. In your example, it would be if you hover over testeval somewhere later in your code where you're using it. For my example, it's when I hover over the workflow variable later in my code.
And that includes if I add that note you posted to both my project and user-level settings.json
Comment From: firelizzard18
Yeah, that matches what I saw. While I'm confident that implementing your feature would involve tweaking the Hover function, it's not clear to me what the code path is for hovering on a package identifier (e.g. testeval) vs a package import, which is why I said I'm not certain what needs to change.
Comment From: findleyr
This is quite old logic, and it looks like https://go.dev/cl/230417 just added special handling for import specs. The case of a PkgName object in the code falls back to default behavior for object references.
We could certainly do this, if people would find it helpful. I will move this to the gopls issue tracker as a feature request.
Comment From: firelizzard18
@findleyr Are you thinking to use the same verbosity level for the import hover and the package identifier hover? If that's the case it seems like it wouldn't be too hard for an external contributor to implement.
Comment From: findleyr
@firelizzard18 yes, I should have been clearer: I think it would be reasonable to serve the same hover content for the package name in a qualified identifier as we do for an import spec. If anything, the lack of consistency is surprising. Given a bit of historical digging, I don't think this was intentional.
Comment From: GSmithApps
Yeah same here — I also was surprised and couldn’t determine if I had misconfigured something.
I can take a stab at implementing it, but someone else would probably be much faster. I see firelizzard's link and can poke around in here, but if anyone has a recommendation on where to start, I’m all ears.
Thanks again @firelizzard18 and @findleyr !
Comment From: firelizzard18
@GSmithApps It will probably be fastest for you to implement it. I'm interested myself but I have multiple in-progress CLs and I don't plan on opening any more until those are merged, which may take some time. Personally I'd start with using tests and the debugger to step through the code to understand how it flows and how it might be changed. TestHoverImport tests hovering an import URL; for hovering a package identifier I wrote the test below (copied from TestHoverImport) and it appears to work as expected though it currently fails since Hover does not return the package documentation.
TestHoverPackageIdent
func TestHoverPackageIdent(t *testing.T) {
const packageDoc1 = "Package lib1 hover documentation"
const packageDoc2 = "Package lib2 hover documentation"
tests := []struct {
hoverIdent string
want string
wantError bool
}{
{
"lib1",
packageDoc1,
false,
},
{
"lib2",
packageDoc2,
false,
},
{
"lib3",
"",
false,
},
{
"lib4",
"",
true,
},
}
source := fmt.Sprintf(`
-- go.mod --
module mod.com
go 1.12
-- lib1/a.go --
// %s
package lib1
const C = 1
-- lib1/b.go --
package lib1
const D = 1
-- lib2/a.go --
// %s
package lib2
const E = 1
-- lib3/a.go --
package lib3
const F = 1
-- main.go --
package main
import (
"mod.com/lib1"
"mod.com/lib2"
"mod.com/lib3"
"mod.com/lib4"
)
func main() {
println(lib1.C)
println(lib2.E)
println(lib3.F)
println(lib4.Z)
}
`, packageDoc1, packageDoc2)
Run(t, source, func(t *testing.T, env *Env) {
env.OpenFile("main.go")
for _, test := range tests {
got, _, err := env.Editor.Hover(env.Ctx, env.RegexpSearch("main.go", "("+test.hoverIdent+")\\."))
if test.wantError {
if err == nil {
t.Errorf("Hover(%q) succeeded unexpectedly", test.hoverIdent)
}
} else if !strings.Contains(got.Value, test.want) {
t.Errorf("Hover(%q): got:\n%q\nwant:\n%q", test.hoverIdent, got.Value, test.want)
}
}
})
}
Comment From: GSmithApps
Oh wow MVP status @firelizzard18 thank you so much!
I went ahead and forked the repo (here) and added your test and added you two as contributors in case you get some inspiration.
Thanks again -- hopefully we/I can get this up and running!
Comment From: GSmithApps
Okay I think we've found the first point at which the existing test (for hovering over the import statement) and the second test (to test the identifier) differ. It's in hover.go. The left side of the image is the old test, and it gets caught in this if statement, but the new test doesn't get caught in there. This is because this if statement is for imports.
So I think what we need to do is figure out where in the remainder of that function we want it to hit for the identifier test 👍
Comment From: firelizzard18
You'll probably need to refactor hoverImport since it takes a *ast.ImportSpec as a parameter which you won't have when handling a package identifier. Maybe move the common code to a new function, hoverPackageRef, with hoverImport calling that and a new function hoverPackageIdent that also calls hoverPackageRef. Then if you add the following after ident, obj, selectedType := referencedObject(pkg, pgf, pos) it should do the trick.
if _, ok := obj.(*types.PkgName); ok {
rng, hoverRes, err := hoverPackageIdent(ctx, snapshot, pkg, pgf, ident)
if err != nil {
return protocol.Range{}, nil, err
}
if hoverRange == nil {
hoverRange = &rng
}
return *hoverRange, hoverRes, nil // (hoverRes may be nil)
}
I pretty much just copied that from the hoverImport case so you may have to change it.
Comment From: GSmithApps
Okay awesome -- thank so much again @firelizzard18 !
I went ahead and copy-pasted in what you suggested and did a little bit of tweaking, but it's not quite there yet. I think I/we'll have to look at it again one more time tomorrow or something
(Here's the repo) again and some screenshots
Comment From: findleyr
@GSmithApps please send me the CL when it is ready for review, thanks!
Comment From: GSmithApps
Hey @findleyr here's the pull request and CL
Ethan's test is passing (screenshot below), and the implementation is pretty close to what he suggested
