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

[READY] Migrate to GOPLS #1243

Merged
merged 2 commits into from Jun 2, 2019
Merged

[READY] Migrate to GOPLS #1243

merged 2 commits into from Jun 2, 2019

Conversation

bstaletic
Copy link
Collaborator

@bstaletic bstaletic commented May 8, 2019

It took a while, but it should work now. The gopls commit is pinned, because go get -u breaks daily.

Supports:

  • Completion
  • Diagnostics
  • Format (but only whole file)
  • GoTo
  • GoToDefintion
  • GoToDeclaration (via GoToDefinition)
  • GoToType
  • RestartServer
  • FixIt*

Limitations:

  • If any parent directory is named test_data, gopls doesn't work. This is a feature!
  • If the file being edited isn't in a module or in the $GOPATH, gopls won't be able to parse it.
  • If the file is inside a module directory or inside $GOPATH but doesn't obey domain/owner/project directory hierarchy, gopls won't work.
  • If the hierarchy is correct, but the file that declares package foo isn't in directory domain/owner/project/foo, gopls won't work.

This is all in line with the standard golang project structure, but it means that a quick vim foo.go is impossible.

Fixes #1217

* Fixits behave od. gopls really wants to format the code even if it is unrelated to the actual FixIt reported. (check what the Subocommands_FixIt_Simple_test() is doing).


This change is Reviewable

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the amount of code this removes.

But I don't love the limitations. As is normal for me, I use go in literally ONE project. I'd like to see if this works with that project as a sample-of-one test.

Reviewed 25 of 25 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)


.gitmodules, line 46 at r1 (raw file):

	path = third_party/requests_deps/idna
	url = https://github.com/kjd/idna
[submodule "third_party/go/src/golang.org/x/tools"]

what does the 'x' mean ?


ycmd/completers/go/go_completer.py, line 90 at r1 (raw file):

  def GetType( self, request_data ):
    # Quite stupidly, gopls returns this as markdown

Do we correctly declare the supported types of markup? IIRC there is/was a bug that we declared it wrong, that I fixed, but may or may not have actually pushed the fix for. But ultimately, markdown is better than HTML or some other nonsense.

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I don't love the limitations. As is normal for me, I use go in literally ONE project. I'd like to see if this works with that project as a sample-of-one test.

This sounds very reasonable, though keep in mind that the conversion to modules is easy, provided that the project always obeyed the standard directory hierarchy.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)


.gitmodules, line 46 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

what does the 'x' mean ?

That's the "owner" in "domain/owner/project" I mentioned. It's where golang keeps a bunch of repos: https://godoc.org/-/subrepo

Needs to match the upstream repo hierarchy so we can build anything from the tools with just go build and avoid fiddling with $GOPATH. (Not to mention that the old way of setting $GOPATH somehow failed to work.


ycmd/completers/go/go_completer.py, line 90 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Do we correctly declare the supported types of markup? IIRC there is/was a bug that we declared it wrong, that I fixed, but may or may not have actually pushed the fix for. But ultimately, markdown is better than HTML or some other nonsense.

This is a stale comment. GOPLS now obeys that we didn't declare the markdown capability. I.e. it returns plain text.

Comment removed.

@codecov
Copy link

codecov bot commented May 10, 2019

Codecov Report

Merging #1243 into master will decrease coverage by 0.01%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #1243      +/-   ##
==========================================
- Coverage   97.42%   97.41%   -0.02%     
==========================================
  Files          95       95              
  Lines        7389     7262     -127     
  Branches      153      153              
==========================================
- Hits         7199     7074     -125     
+ Misses        149      147       -2     
  Partials       41       41

@bstaletic
Copy link
Collaborator Author

Turns out gopls supports FixIts, so added support for that.

Though, it behaves weird. It really wants to format the code even if it is unrelated to the actual FixIt reported. (check what the Subocommands_FixIt_Simple_test() is doing).

@rajathagasthya
Copy link

Any ETA on this? Looking forward to trying out Gopls with YCM.

@bstaletic
Copy link
Collaborator Author

bstaletic commented May 20, 2019

You can always merge it locally. ETA is whenver it gets reviewed.

To merge it locally, you need to do the following:

  • cd YouCompleteMe/third_party/ycmd
  • git fetch origin +refs/pull/1243/merge:gopls
  • git checkout gopls
  • git submodule update --init --recursive
  • ./build.py --skip-build --go-completer

@orlangure
Copy link

To merge it locally, you need to do the following:

@bstaletic, this does not seem to work, I'm still seeing gocode processes started by the plugin. Do I need to add anything to configuration to make the switch?

@mjolk
Copy link

mjolk commented May 20, 2019

@orlangure after git fetch ... you need to run git checkout gopls, then run git submodule update ... etc
@bstaletic great work! thanks!

@orlangure
Copy link

after git fetch ... you need to run git checkout gopls, then run git submodule update

@mjolk, thanks, I see now that the original comment was also edited.
@bstaletic works great, thank you!

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Deletes a LOAD of code
  • Seems to be strong community support
  • Re-uses the language server implementation
  • Tests are passing
  • Code looks good.

What's not to like.

:lgtm:

Reviewed 5 of 5 files at r3.
Reviewable status: 1 of 2 LGTMs obtained

@preslavmihaylov
Copy link

This is great. I just ran into the problem fixed by this MR and was about to open a new issue.

Can't wait for the final LGTM to approve this and get it merged.

@puremourning puremourning added the Ship It! Manual override to merge a PR by maintainer label Jun 2, 2019
@puremourning
Copy link
Member

Ship It.

Multiple users have expressed interest in this PR and I have reviewed it. It net reduces our code and thus liability and increases functionality for users.

@mergify
Copy link
Contributor

mergify bot commented Jun 2, 2019

Thanks for sending a PR!

Copy link
Collaborator Author

@bstaletic bstaletic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased and fixed the tests after rebase.

Reviewable status: 0 of 2 LGTMs obtained (and 1 stale) (waiting on @puremourning)

Copy link
Member

@puremourning puremourning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 6 of 6 files at r4.
Reviewable status: 1 of 2 LGTMs obtained

@mergify mergify bot merged commit fc7e758 into ycm-core:master Jun 2, 2019
@mergify
Copy link
Contributor

mergify bot commented Jun 2, 2019

Thanks for sending a PR!

@bstaletic bstaletic deleted the gopls branch June 2, 2019 18:49
@bstaletic
Copy link
Collaborator Author

For everyone watching this thread... I messed up the rebase and we have merged a broken completer. #1257 fixes this and it's literally a two character fix.

bstaletic pushed a commit to bstaletic/YouCompleteMe that referenced this pull request Jul 3, 2019
ycm-core/ycmd#1275 Remap python GoTo* commands
ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn
ycm-core/ycmd#1239 Error ID in diagnostic messages
ycm-core/ycmd#1267 Clangd extra conf support
ycm-core/ycmd#1266 JDT update to 0.40.0
ycm-core/ycmd#1245 Generic (pluggable) LSP completer support
ycm-core/ycmd#1263 Support JDT extensions
ycm-core/ycmd#1261 Typescript update
ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration
ycm-core/ycmd#1262 Multi-user installation
ycm-core/ycmd#1243 Migrate to GOPLS
ycm-core/ycmd#1249 Support MSVC 16 (VS2019)
ycm-core/ycmd#1224 Migrate to RLS
bstaletic pushed a commit to bstaletic/YouCompleteMe that referenced this pull request Jul 4, 2019
ycm-core/ycmd#1275 Remap python GoTo* commands
ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn
ycm-core/ycmd#1239 Error ID in diagnostic messages
ycm-core/ycmd#1267 Clangd extra conf support
ycm-core/ycmd#1266 JDT update to 0.40.0
ycm-core/ycmd#1245 Generic (pluggable) LSP completer support
ycm-core/ycmd#1263 Support JDT extensions
ycm-core/ycmd#1261 Typescript update
ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration
ycm-core/ycmd#1262 Multi-user installation
ycm-core/ycmd#1243 Migrate to GOPLS
ycm-core/ycmd#1249 Support MSVC 16 (VS2019)
ycm-core/ycmd#1224 Migrate to RLS
bstaletic pushed a commit to bstaletic/YouCompleteMe that referenced this pull request Jul 4, 2019
ycm-core/ycmd#1275 Remap python GoTo* commands
ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn
ycm-core/ycmd#1239 Error ID in diagnostic messages
ycm-core/ycmd#1267 Clangd extra conf support
ycm-core/ycmd#1266 JDT update to 0.40.0
ycm-core/ycmd#1245 Generic (pluggable) LSP completer support
ycm-core/ycmd#1263 Support JDT extensions
ycm-core/ycmd#1261 Typescript update
ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration
ycm-core/ycmd#1262 Multi-user installation
ycm-core/ycmd#1243 Migrate to GOPLS
ycm-core/ycmd#1249 Support MSVC 16 (VS2019)
ycm-core/ycmd#1224 Migrate to RLS
bstaletic pushed a commit to bstaletic/YouCompleteMe that referenced this pull request Jul 4, 2019
ycm-core/ycmd#1275 Remap python GoTo* commands
ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn
ycm-core/ycmd#1239 Error ID in diagnostic messages
ycm-core/ycmd#1267 Clangd extra conf support
ycm-core/ycmd#1266 JDT update to 0.40.0
ycm-core/ycmd#1245 Generic (pluggable) LSP completer support
ycm-core/ycmd#1263 Support JDT extensions
ycm-core/ycmd#1261 Typescript update
ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration
ycm-core/ycmd#1262 Multi-user installation
ycm-core/ycmd#1243 Migrate to GOPLS
ycm-core/ycmd#1249 Support MSVC 16 (VS2019)
ycm-core/ycmd#1224 Migrate to RLS
bstaletic pushed a commit to bstaletic/YouCompleteMe that referenced this pull request Jul 5, 2019
ycm-core/ycmd#1275 Remap python GoTo* commands
ycm-core/ycmd#1274 Migrate to Omnisharp-Roslyn
ycm-core/ycmd#1239 Error ID in diagnostic messages
ycm-core/ycmd#1267 Clangd extra conf support
ycm-core/ycmd#1266 JDT update to 0.40.0
ycm-core/ycmd#1245 Generic (pluggable) LSP completer support
ycm-core/ycmd#1263 Support JDT extensions
ycm-core/ycmd#1261 Typescript update
ycm-core/ycmd#1264 [vimspector](/puremourning/vimspector) configuration
ycm-core/ycmd#1262 Multi-user installation
ycm-core/ycmd#1243 Migrate to GOPLS
ycm-core/ycmd#1249 Support MSVC 16 (VS2019)
ycm-core/ycmd#1224 Migrate to RLS
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ship It! Manual override to merge a PR by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate Go completer to gopls
6 participants