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: show hover information for runes #38239

Open
stamblerre opened this issue Apr 3, 2020 · 6 comments
Open

x/tools/gopls: show hover information for runes #38239

stamblerre opened this issue Apr 3, 2020 · 6 comments

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Apr 3, 2020

From microsoft/vscode-go#3149:

In Go, runes can be expressed as single-quoted readable glyphs, single-quoted escaped \u codes, or number literals (usually hex). It’s often hard to know what a particular rune is, short of extensive commenting as in this example.

It would be very helpful in VSCode to hover over any of these forms to get info on the rune, such as:

  • visual glyph (if any)
  • description e.g. “ARABIC THOUSANDS SEPARATOR”
  • \u representation
  • hex representation

Thanks for the consideration, happy to discuss.

This can be implemented using https://pkg.go.dev/golang.org/x/text/unicode/runenames.

@jhchabran
Copy link

@jhchabran jhchabran commented Apr 30, 2021

If this ticket is still up to date, I'd like to implement it.

Is that okay if I take it @stamblerre?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented Apr 30, 2021

Absolutely, @jhchabran! Please comment here if you need any additional guidance.

@jhchabran
Copy link

@jhchabran jhchabran commented May 3, 2021

Thanks! I think I need some guidance regarding the marker tests.

I wrote some regtests for hovering runes and got them green, but I am unsure how to proceed with the marker tests or even if I should write some for this feature.

From what I understand, roughly speaking, the @hover("something", mark) works by matching something and checking that hovering it references the object that was marked.

Hovering a rune does not reference anything, so I don't see how I could express that with a marker test, assuming I understood how it presently works. Maybe some self-reference?

Do you think I should dig into the markers more and eventually see how they could be made to work with the rune hovering or the regtests would be enough for such an "isolated" feature?

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented May 3, 2021

Thanks for working on this! The regtests might be sufficient for this case, but if you'd like to add marker-based tests, you could also adjust the way that the @hover test works, here: https://github.com/golang/tools/blob/062bf4eb8ab9c68e2c22214899f3e17b66ddbbf3/internal/lsp/tests/tests.go#L437.

@jhchabran
Copy link

@jhchabran jhchabran commented May 14, 2021

Thanks for the pointer! I spent more time on the markers and got how they work.
It now basically looks like this:

// internal/lsp/testdata/basiclit/baciclit.go
// (...) 
_ = '\U0001F30A' //@hovertooltip("'\\U0001F30A'", "'🌊', U+1F30A, WATER WAVE")

Another marker called @hovertooltip was added because modifying the @hover to make it work with expecting a reference to a definition or a direct string representation of the expected result is probably too convoluted and unclear to read.

➡️ Is that okay to have added another one? I think it makes sense especially if there are other hovers that may be added later on.

I also get the feeling that renaming @hover to @hoverdef would make more sense now, but that's probably better to discuss it over a CL rather than here.

@stamblerre
Copy link
Contributor Author

@stamblerre stamblerre commented May 14, 2021

Yep, totally fine to add a new marker type! Happy to continue the conversation over a CL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants