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

When gotoSymbol.includeGoroot is set, also show symbols from GOROOT #1604

Merged
merged 5 commits into from
Apr 5, 2018
Merged

When gotoSymbol.includeGoroot is set, also show symbols from GOROOT #1604

merged 5 commits into from
Apr 5, 2018

Conversation

m90
Copy link
Contributor

@m90 m90 commented Apr 3, 2018

Setting the option to true enables the user to include the standard
library located at GOROOT to be included in the results shown on
"Go To Symbol" - resolves #1567

m90 added 2 commits April 3, 2018 12:09
Setting the option to true enables the user to include the standard
library located at GOROOT to be included in the results shown on
"Go To Symbol" - resolves #1567
src/goSymbol.ts Outdated
return symbols;
});
let workspaceSymbols = getSymbols(root, query, token, goConfig);
let gorootSymbols = goConfig.gotoSymbol.includeGoroot && process.env.GOROOT
Copy link
Contributor

Choose a reason for hiding this comment

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

process.env.GOROOT will be undefined unless the user has set the env var or added the go.goroot setting.
On my Mac, its neither.
We need to run go env GOROOT to get the goroot.
See https://github.com/Microsoft/vscode-go/blob/0.6.78/src/goInstallTools.ts#L298 for reference

test/go.test.ts Outdated
@@ -703,11 +703,11 @@ It returns the number of bytes written and any write error encountered.
}
}
});
let withoutIgnoringFolders = getWorkspaceSymbols(workspacePath, 'WinInfo', null, configWithoutIgnoringFolders).then(results => {
let withoutIgnoringFolders = getSymbols(workspacePath, 'WinInfo', null, configWithoutIgnoringFolders).then(results => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update this test case and add 2 cases for with and without including goroot just like we do here for with and without ignoring folders?

Copy link
Contributor Author

@m90 m90 Apr 4, 2018

Choose a reason for hiding this comment

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

The function that is being tested here does not know anything about the logic that is being introduced in this PR. If we wanted to add test cases for the behavior we would need add 2 full "integration-style" tests for #provideWorkspaceSymbols, which is currently not being tested at all. I would be happy to add these, but wanted to double check if this is what you are talking about?

Alternatively it might also be an option to move the logic that is introduced here into this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant something like this: bbf329c

Copy link
Contributor Author

@m90 m90 Apr 4, 2018

Choose a reason for hiding this comment

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

But does this change in config have any effect on getSymbols? The check it wants to test is currently done higher up in the call hierarchy if I'm not mistaken.

@m90
Copy link
Contributor Author

m90 commented Apr 4, 2018

@ramya-rao-a I am slightly confused about how to continue here. The tests you added do not match the way the code works (they fail on linting right now but would fail once the results are added to the Promise.all underneath). We could fix this by either adjusting the tests according to the way the code works, or we refactor the code to match the test cases you added. I'm not sure.

Let me know how to proceed.

test/go.test.ts Outdated
let withIncludingGoroot = getSymbols(workspacePath, 'WinInfo', null, configWithIncludeGoroot).then(results => {
assert.equal(results[0].name, 'printf');
});

Promise.all([withIgnoringFolders, withoutIgnoringFolders]).then(() => done(), done);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would still need to include the test cases generated above in order to make the test fail or pass.

@ramya-rao-a
Copy link
Contributor

Ah! sorry about that.
I've pushed another commit that will test the symbol provider directly

@ramya-rao-a
Copy link
Contributor

The test still fails, I didnt have the time to debug it yet. Can you take a look?

@m90
Copy link
Contributor Author

m90 commented Apr 4, 2018

I actually think it might be nicer to do it the way your intuition and the tests you wrote were suspecting and move the logic of potentially calling twice down a method. I will force push over your changes for doing that so I can reuse the tests you initially added.

@m90
Copy link
Contributor Author

m90 commented Apr 4, 2018

@ramya-rao-a I refactored the code so that your initial testing approach works now, and I think it's a lot nicer that way as all the logic on the gotoSymbolConfig happens in the same place. Tests are passing and I'd be happy.

Let me know if you have further remarks.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @m90!

@ramya-rao-a ramya-rao-a merged commit c0a489f into microsoft:master Apr 5, 2018
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.

Include the standard library in 'Go to Symbol in Workspace...' searching
2 participants