fix: #2309 use goimports instead of goreturns when using modules

[jamesgeorge007]

Closes #2309

• Updated goFormat.ts to use goimports if user's choice is goreturns in module mode

[jamesgeorge007]

@ramya-rao-a What do you think?

[ramya-rao-a]

isModSupported does not return a boolean. It returns a promise that resolves to a boolean. Look at other instances where this function was used to see the proper usage

[jamesgeorge007]

@ramya-rao-a Is this fine 🤔

[jamesgeorge007]

@lloiser I'm not sure if this is the right implementation. Kindly let me know about the required changes to be made further 👍

[lloiser]

both the issue and the description of your pull request say:

use goimports if user's choice is goreturns in module mode

But this will effectivly overwrite formatTool also if the value is gofmt.

It's also unnecessary to call isModSupported if the formatTool is already goimports.

[jamesgeorge007]

@lloiser I'm not sure how do you intend to invoke isModSupported as per the above mentioned case.
Do you mean something like checking for whether formatTool is goimports and invoking the method appropriately 🤔

if (formatTool === 'goimports') {
 //
}

I'm not familiar with the codebase as mentioned before. Kindly suggest the required changes 😄

[ramya-rao-a]

If the chosen tool is either gofmt or goimports, then no change is needed.
Else, you need to invoke isModSupported to figure out if modules are being used or not.
Something like the below

private getFormatTool(goConfig: vscode.WorkspaceConfiguration, fileUri: vscode.Uri): Thenable<string> {
	let formatTool = goConfig["formatTool"] || "goreturns";
	if (formatTool === "gofmt" || formatTool === "goimports") {
		return Promise.resolve(formatTool);
	}
	return isModSupported(fileUri).then(isMod => {
		return isMod ? "goimports": formatTool;
	})
} 

On second thoughts, I wonder if we should choose to use gofmt instead. gofmt is installed by default for all, whereas one has to specifically install goimports. Also, older versions of goimports don't support modules, so goimports has to be installed, meaning we need to prompt the user to do so.

Regardless, we are choosing to use a different tool than what the user meant to use. Therefore, we should notify the user of this change, and update workspace settings so that we don't have to make this check every time the file needs to be formatted.

For reference, this updating of workspace settings when using modules is already being done for the go.infergopath setting at https://github.com/Microsoft/vscode-go/blob/0.9.2/src/goModules.ts#L55. We can update the same

[jamesgeorge007]

@ramya-rao-a Is this the way of implementation that you intended 🤔

[jamesgeorge007]

@lloiser Hope it's fine now 🤔

[ramya-rao-a]

@jamesgeorge007, @lloiser I figured a simpler way to use goimports when using modules that doesnt involve checking isModSupported for each formatting request. See 81013f5

Therefore, am closing this PR. Thanks for all the effort here.