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

Add support for gopls 'implements' functionality #2741

Merged
merged 2 commits into from
Mar 29, 2020

Conversation

woodstok
Copy link

Add support to use the textDocument/implementation gopls feature.
Add g:go_implements_mode so that users can choose between gopls and
guru.

Add support to use the textDocument/implementation gopls feature.
Add g:go_implements_mode so that users can choose between gopls and
guru.
@woodstok
Copy link
Author

In addition to the default Implements feature,
https://go-review.googlesource.com/c/tools/+/219678/ is in the works which would bring a reverse Implements functionality. Hopefully that merges soon to gopls

Copy link
Collaborator

@bhcleek bhcleek left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! This looks great.

I think that CL for reverse implements is really what's needed, because that's what :GoImplements means now.

I'm a little reluctant to overload :GoImplements to do what this PR does, but as a practical matter it probably makes sense. I'm going to give it some more thought before merging, though.

Please ignore all my comments on the PR; they're really for me to reference after merging to do some small cleanup.

LGTM

@@ -151,6 +151,7 @@ let s:default_list_type_commands = {
\ "GoTest": "quickfix",
\ "GoVet": "quickfix",
\ "GoReferrers": "quickfix",
\ "GoImplements": "quickfix",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The default for GoImplements should be locationlist, because the information that it the list will contain is relative to a single buffer.

" a dictionary that manages state (statuslines, sets the winid, etc.). handler
" should take three arguments: an exit_code, a JSON object encoded to a string
" that mimics guru's ouput for `what`, and third mode parameter that only
" exists for compatibility with the guru implementation of SameIDs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

(nit): SameIDS -> implements

There's no need for you to change this; I'm just leaving a note for myself to fix this after merging.


let l:state = s:newHandlerState('implements')

let l:state.handleResult = funcref('s:referencesHandler', [function(a:handler, [], l:state)], l:state)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just another note for myself to do later: s:referencesHandler should be refactored to call another function. This should call a new function s:implementsHandler, which calls that common function.

endfunction

function! s:implementsErrorHandler(next, error) abort dict
call call(a:next, [-1, [a:error], ''])
Copy link
Collaborator

Choose a reason for hiding this comment

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

-1 is ok, but 1 may be more consistent with other exit codes.


let errformat = ",%f:%l:%c:\ %m"
let l:listtype = go#list#Type("GoImplements")
call go#list#ParseFormat(l:listtype, errformat, a:output, '')
Copy link
Collaborator

Choose a reason for hiding this comment

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

The last argument should be a:title.

@bhcleek
Copy link
Collaborator

bhcleek commented Feb 29, 2020

@woodstok do you mind if I push to your branch before merging?

@woodstok
Copy link
Author

@bhcleek Feel free

@woodstok
Copy link
Author

woodstok commented Mar 4, 2020

In addition to the default Implements feature,
https://go-review.googlesource.com/c/tools/+/219678/ is in the works which would bring a reverse Implements functionality. Hopefully that merges soon to gopls

This has merged to gopls master.
@bhcleek, just to clarify, the review comments were for your reference right? Or were you waiting for me to make the changes?

@bhcleek
Copy link
Collaborator

bhcleek commented Mar 4, 2020

Thanks for checking in. I'm planning to make the changes, I've just been very busy lately

@bhcleek
Copy link
Collaborator

bhcleek commented Mar 7, 2020

@woodstok given how this works now, I think it might also be a good replacement for :GoCallees. WDYT?

@woodstok
Copy link
Author

@woodstok given how this works now, I think it might also be a good replacement for :GoCallees. WDYT?

I am not sure if they map to the same functionality. And I can't seem to get my go guru to work properly in my projects, so can't verify either.

@bhcleek
Copy link
Collaborator

bhcleek commented Mar 14, 2020

Guru works in GOPATH mode. For GoCallees, you may need to call :GoGuruScope with the scope that you want to evaluate.

@bhcleek
Copy link
Collaborator

bhcleek commented Mar 23, 2020

Thank you for your patience @woodstok . Now that I have a few bugs off my plate, I'm going to focus on getting this merged.

I'll be pushing to your branch soon as we previously discussed.

* use the location list by default for :GoImplements, because it
  operates only on a single file (though the results may be for multiple
  files).
* use the location list by default for :GoReferrers, because it operates
  only on a single file (though the results may be for multiple files).
* make sure the location list for :GoImplements has title.
* use separate handlers for go#lsp#Implements and go#lsp#Referrers.
* rename s:referencesHandler in lsp.vim to s:handleLocations and call it
  from s:handleReferences and s:handleImplements.
* Adjust documentation for go#lsp#Implementation to refer to guru
  implements instead of guru what.
* Document the default for g:go_implements_mode and g:go_referrers_mode.
@bhcleek bhcleek added this to the vim-go 1.23 milestone Mar 29, 2020
@bhcleek bhcleek merged commit 2d9460e into fatih:master Mar 29, 2020
bhcleek added a commit that referenced this pull request Mar 29, 2020
@bhcleek
Copy link
Collaborator

bhcleek commented Mar 29, 2020

Thank you again for the contribution 🙇

@woodstok
Copy link
Author

Thank you for your patience @woodstok . Now that I have a few bugs off my plate, I'm going to focus on getting this merged.

I'll be pushing to your branch soon as we previously discussed.

Thank you for the merge. I was away from my laptop for quite a while.

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

Successfully merging this pull request may close these issues.

3 participants