When gotoSymbol.includeGoroot is set, also show symbols from GOROOT - #1604
Conversation
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
| return symbols; | ||
| }); | ||
| let workspaceSymbols = getSymbols(root, query, token, goConfig); | ||
| let gorootSymbols = goConfig.gotoSymbol.includeGoroot && process.env.GOROOT |
There was a problem hiding this comment.
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
| } | ||
| }); | ||
| let withoutIgnoringFolders = getWorkspaceSymbols(workspacePath, 'WinInfo', null, configWithoutIgnoringFolders).then(results => { | ||
| let withoutIgnoringFolders = getSymbols(workspacePath, 'WinInfo', null, configWithoutIgnoringFolders).then(results => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Ramya Rao (@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 Let me know how to proceed. |
| assert.equal(results[0].name, 'printf'); | ||
| }); | ||
|
|
||
| Promise.all([withIgnoringFolders, withoutIgnoringFolders]).then(() => done(), done); |
There was a problem hiding this comment.
This would still need to include the test cases generated above in order to make the test fail or pass.
|
Ah! sorry about that. |
|
The test still fails, I didnt have the time to debug it yet. Can you take a look? |
|
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. |
|
Ramya Rao (@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 Let me know if you have further remarks. |
Ramya Rao (ramya-rao-a)
left a comment
Ramya Rao (ramya-rao-a)
left a comment
There was a problem hiding this comment.
Looks great, thanks Frederik Ring (@m90)!
2 participants
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