Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Use gogetdoc instead of godef and godoc #622

Merged
merged 11 commits into from
Nov 20, 2016

Conversation

ramya-rao-a
Copy link
Contributor

@ramya-rao-a ramya-rao-a commented Nov 17, 2016

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

  • A setting where user can choose godoc (old way of doing things godef + godoc) vs gogetdoc
  • If User is using Go 1.5, then we use the old way regardless of what is set
  • The setting will default to the old way. There are some 3 or 4 issues logged by users which using gogetdoc will solve. We will reach out to those users and ask them to change the setting to use gogetdoc and give feedback
  • Some refactoring
  • Fix the failing tests run by travis
  • Add unit tests to cover the scenarios mentioned in the above mentioned issues

@ramya-rao-a
Copy link
Contributor Author

ramya-rao-a commented Nov 17, 2016

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
Copy link
Contributor

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

Copy link
Contributor Author

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);
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@ramya-rao-a ramya-rao-a Nov 18, 2016

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.

Copy link
Contributor Author

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

@ramya-rao-a ramya-rao-a force-pushed the gogetdoc-godoc branch 2 times, most recently from d2127d3 to d0c6671 Compare November 17, 2016 18:02
@ramya-rao-a
Copy link
Contributor Author

All tests are a pass!

@ramya-rao-a
Copy link
Contributor Author

Added additional tests to cover various scenarios where using gogetdoc wins vs godef

// 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') {
Copy link
Contributor

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') { :)

Copy link
Contributor Author

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...

@zmb3
Copy link
Contributor

zmb3 commented Nov 18, 2016

LGTM :-)

@abarisain
Copy link
Contributor

Ditto

@ramya-rao-a ramya-rao-a merged commit 0a14202 into microsoft:master Nov 20, 2016
@abarisain
Copy link
Contributor

Great addition to the plugin ! Congrats @zmb3

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

Successfully merging this pull request may close these issues.

5 participants