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: improve textDocument/documentLink #33505

Closed
stamblerre opened this issue Aug 6, 2019 · 11 comments
Closed

x/tools/gopls: improve textDocument/documentLink #33505

stamblerre opened this issue Aug 6, 2019 · 11 comments
Labels
Documentation FrozenDueToAge gopls Issues related to the Go language server, gopls. help wanted NeedsFix The path to resolution is known, but the work has not been done.

Comments

@stamblerre
Copy link
Contributor

stamblerre commented Aug 6, 2019

Some additional features we should add support for in textDocument/documentLink:

  • Links missing http:// or https:// should be treated as links. For example, golang.org/issue/12345. (Completed via golang.org/cl/194661).
  • Links to the Go issue tracker should be treated as links. For example, golang/go#12345.

Please add any others as ideas come up.

@stamblerre stamblerre added NeedsFix The path to resolution is known, but the work has not been done. gopls Issues related to the Go language server, gopls. labels Aug 6, 2019
@gopherbot gopherbot added this to the Unreleased milestone Aug 6, 2019
@stamblerre stamblerre added Suggested Issues that may be good for new contributors looking for work to do. and removed Documentation labels Aug 6, 2019
@stamblerre stamblerre added help wanted and removed Suggested Issues that may be good for new contributors looking for work to do. labels Aug 8, 2019
@eduvim
Copy link

eduvim commented Aug 13, 2019

I'm working on it

@gopherbot
Copy link

Change https://golang.org/cl/194537 mentions this issue: internal/links: Improve links parser, no protocol specification

@gopherbot
Copy link

Change https://golang.org/cl/194661 mentions this issue: internal/links: Improve links parser, no protocol specification

@stamblerre
Copy link
Contributor Author

Re-opening because not all of these features have been implemented.

@stamblerre stamblerre reopened this Dec 3, 2019
@eduvim
Copy link

eduvim commented Dec 3, 2019

what we can do is to check at links file if the link is http | https or nothing followed by golang/go#number . I can help with it. I just need to understand it in order to don't make a lot of pr's xD

@stamblerre
Copy link
Contributor Author

@eduvim: If you'd like to continue work on this, I think we could add a second regex to match on golang/go#1234. Please feel free to add any other ideas as they come up.

@stamblerre stamblerre modified the milestones: Unreleased, gopls unplanned Dec 4, 2019
@zikaeroh
Copy link
Contributor

zikaeroh commented Dec 4, 2019

Is there any chance that the hueristic can be disabled without also disabling import links? I'm finding the current changes pretty distracting when it thinks things are links which actually aren't, e.g.:

image

I'd personally prefer this feature to not just accept anything that's foo.bar and start treating it as links in strings/comments.

And, if you hadn't seen it, there's already some great work in this area by @mvdan I use in another project: https://github.com/mvdan/xurls

@stamblerre
Copy link
Contributor Author

Thank you for the suggestion @zikaeroh! I would be in favor of us using this library to do a better job of detecting links. What do you think, @ianthehat?

@gopherbot
Copy link

Change https://golang.org/cl/212517 mentions this issue: gopls: use mvdan.cc/xurls for textDocument/documentLink

@eduvim
Copy link

eduvim commented Dec 26, 2019

Would you need any help ?

gopherbot pushed a commit to golang/tools that referenced this issue Dec 26, 2019
Our current implementation isn't robust, and it doesn't seem worth it to
invest significant effort in improving it when this library exists.

Also, make the protocol part of the default URL regex non-optional, as
the alternative is that any string of the format "foo.bar" will appear
to be a link.

Updates golang/go#33505

Change-Id: Ia430a1c193eded394f8af12050bdd4dc2a9ccc94
Reviewed-on: https://go-review.googlesource.com/c/tools/+/212517
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@stamblerre
Copy link
Contributor Author

Thank you @eduvim -- I'm sorry I didn't reply earlier, but I actually did a small refactoring of the code for textDocument/documentLink which resolved this issue. I think I will close this, but if you are still interested in contributing, please check out the available issues here.

@golang golang locked and limited conversation to collaborators Dec 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge gopls Issues related to the Go language server, gopls. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
4 participants