-
Notifications
You must be signed in to change notification settings - Fork 645
optimize godef-gomod cache, better support one workspace for multi go… #2216
Conversation
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.
@sunliver Looks like this is your first PR to this project, Thanks & Welcome!
The working directory used to run godef
was intentionally kept to match the current workspace to support the below case:
- Use
Go to definition
on a symbol from an external package - When the file opens (from the module cache) for the above symbol, run
Go to definition
again.
For the second run, godef
expects the working directory to be that of the original file and not of the file from the module cache.
Can you update this PR to ensure the above still holds true?
src/goDeclaration.ts
Outdated
@@ -80,7 +82,7 @@ export function adjustWordPosition(document: vscode.TextDocument, position: vsco | |||
|
|||
const godefImportDefinitionRegex = /^import \(.* ".*"\)$/; | |||
function definitionLocation_godef(input: GoDefinitionInput, token: vscode.CancellationToken): Promise<GoDefinitionInformation> { | |||
let godefTool = input.isMod ? 'godef-gomod' : 'godef'; | |||
let godefTool = 'godef'; |
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.
Let's not change this in this PR. I have a separate issue tracking the need to use godef
instead of godef-mod
, now that the changes from the latter have been merged to the former
src/goDeclaration.ts
Outdated
|
||
return new Promise<GoDefinitionInformation>((resolve, reject) => { | ||
// Spawn `godef` process | ||
p = cp.execFile(godefPath, ['-t', '-i', '-f', input.document.fileName, '-o', offset.toString()], { env, cwd }, (err, stdout, stderr) => { | ||
p = cp.execFile(godefPath, ['-t', '-f', input.document.fileName, '-o', offset.toString()], { env, cwd }, (err, stdout, stderr) => { |
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.
What's the reason for removing the -i
flag?
We had that in place to support the Go to definition feature on unsaved changes in the file
workspaceModCache.forEach((v, k) => { | ||
if (folderPath.startsWith(k)) { | ||
folderModCache.set(folderPath, k); | ||
return Promise.resolve(k); |
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.
This return
applies just to the callback passed to the forEach
call. If you want to exit the getModPath
function when you folderPath.startsWith(k)
is true, then you need to use a simple for
loop
@ramya-rao-a Thanks for you code review! Sorry for some incorrect changes. If I recall right,
|
The first run is when you run |
Actually it runs as you think. If you try to
Why change |
Are you asking why we are using |
But |
@sunliver I have pushed a commit that gets the latest from the master branch. There was a bug where errors from Now going back to the case I was describing in #2216 (comment), lets see the below example
Here if you run Using the code in this PR, this
This is because we are using the folder $GOPATH/pkg/mod as current working directory. |
Closing this PR in favor of #2262 which does something similar and has other fixes |
@ramya-rao-a |
It seems that you may fail back to use workspace folder in some cases.
Now comes back on #2216 (comment), the second run. However, Thus #2216 (comment),
Maybe ask go team for some help. |
see this issue
ChangeLog
go.mod
's path.getModPath
to detect file's mod path. IfgetModPath
returns none empty string, thenisModSupported
returns true.Known Problems
auto
. Because we usego env GOMOD
to findgo.mod
, if may fail when you're under GOPATH project.