-
Notifications
You must be signed in to change notification settings - Fork 645
Use gogetdoc instead of godef and godoc #622
Conversation
5463682
to
755bd27
Compare
755bd27
to
aa102a8
Compare
32aa034
to
aeca9ac
Compare
In travis, the unit tests are failing for 1.7 but pass in 1.6 mac... which is baffling @leaxoy if you have some time to look at the test failures, I'd appreciate it |
return vscode.workspace.openTextDocument(uri).then((textDocument) => { | ||
let promises = testCases.map(([position, expectedSignature, expectedDocumentation]) => | ||
provider.provideHover(textDocument, position, null).then(res => { | ||
// TODO: Documentation appears to currently be broken on Go 1.7, so disabling these tests for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we give reenabling these tests a shot? When this was commented, they worked on my machine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abarisain Done!
text = res.declarationlines[0].substring(5); | ||
si = new SignatureInformation(text, res.doc); | ||
let braceStart = text.indexOf('('); | ||
sig = text.substring(braceStart); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that work for methods that have a signature like func (l *Struct) Add(a int, b int) {
? I did it somewhat differently for that reason https://github.com/abarisain/vscode-go/blob/gogetdoc2/src/goSignature.ts#L33-L40
If it does, then sorry! Haven't had the time to test this branch yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In such case it thinks the receiver is the first param, thats a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed it, Took your code and tested. Will push the changes soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abarisain The changes are in
d2127d3
to
d0c6671
Compare
All tests are a pass! |
ac9beff
to
73dbfc4
Compare
Added additional tests to cover various scenarios where using |
// Install the doc/def tool that was chosen by the user | ||
if (goConfig['docsTool'] === 'godoc') { | ||
tools['godef'] = 'github.com/rogpeppe/godef'; | ||
} else if (goConfig['formatTool'] === 'goreturns') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that should be } else if (goConfig['docsTool'] === 'gogetdoc') {
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I should really stop copy pasting things...
LGTM :-) |
Ditto |
1d16d00
to
8fb8dbc
Compare
Great addition to the plugin ! Congrats @zmb3 |
Currently we use
godef
for Goto/Peek Definition.godef
+godoc
together provide the Hover Info and Signature Help.These tools have shortcomings which we hope to overcome by using
gogetdoc
instead.#440 #442 #515 #567 #459 #496
This PR is work on top of what @leaxoy has done in #607
What has been added are the below
godoc
(old way of doing thingsgodef
+godoc
) vsgogetdoc
gogetdoc
will solve. We will reach out to those users and ask them to change the setting to usegogetdoc
and give feedback