Adding new 'gotests' flags option.

[jeffbean]

gotests has many options not used for vscode by default. This allows
similar flags to other tools like lint and build where they are just
free formed flags.

The one difference here is some arguments have values needed but we are
not validating on any specific flags in preference with flexibility.

The main use case here is the new 'template_dir' flag added to allow
users to define custom templates.

Notable changes around this:

• adding new configuration option genTestsFlags for this new featue.
• change to single main function to switch on the type of tests we want to generate
• adding output to the Go console so debugging is much easier.

[msftclas]

All CLA requirements met.

[ramya-rao-a]

Instead of using this function to pass the goConfig to each of the 3 types of generate functions, and then have them send the flags to the generateTests function, why not access the configuration from inside generateTests directly?

Then, you wouldn't need to create the new GenerateTests function or the GenerationType enum, correct?

[jeffbean]

Yes the goConfig logic was a little dated so I refactored to be a little more concise. I first did this with testing in mind but reworking some of this we can even simplify the case statement to have less overall functions. File and Package were just deciding if it was a file or directory path.

The switch statement seems to be the best way to handle three different commands in the editor. The configuration is not what determines if this is for a File, Package, or Function. This is looking at the goMain.ts in terms of context registration:

	ctx.subscriptions.push(vscode.commands.registerCommand('go.test.generate.package', () => {
		goGenerateTests.GenerateTests(goGenerateTests.GenerationType.Package);
	}));

This seems quite clean compared to the three different functions.

[ramya-rao-a]

Apologies if I am missing something here..., I still dont see the need to have a common GenerateTests or the enum for that matter.

All we need is to access the configuration to get the flags and apply it to gotests. To get configuration we need a uri. So I suggest the below

• The Config object that we create has a dir/file path. Instead lets pass its uri counterpart
• generateTests will now have the uri it can use to call vscode.workspace.getConfiguration to get the flags

[jeffbean]

So the part I think is a misunderstanding is we need more than the configuration object to make the decisions. So there are three seprate commands that do slightly different operations based on the command. The config object will now hold common arguments that will be applied to all three of these seprate commands.

The reason for the single function with an Enum is the cleanest way I found to DRY up this code. I can change back to three seprate function but the addition of a the config object for flags will remain the same.

Having no experience in TypeScript I defer to experts on code style and best practice, but the logic remains that we need a way to run three different commands with similar but different code paths.

   ctx.subscriptions.push(vscode.commands.registerCommand('go.test.generate.package', () => {
		goGenerateTests.GenerateTests(goGenerateTests.GenerationType.Package);
	}));

	ctx.subscriptions.push(vscode.commands.registerCommand('go.test.generate.file', () => {
		goGenerateTests.GenerateTests(goGenerateTests.GenerationType.File);
	}));

	ctx.subscriptions.push(vscode.commands.registerCommand('go.test.generate.function', () => {
		goGenerateTests.GenerateTests(goGenerateTests.GenerationType.Function);
	}));

These callers need a way of knowing what command is being called.

[ramya-rao-a]

we need more than the configuration object to make the decisions

I am assuming you are referring to the VS Code config and not the Config object being created in this file

So there are three seprate commands that do slightly different operations based on the command.

Yes. But this was already covered even before the changes in this PR. If you see https://github.com/Microsoft/vscode-go/blob/0.6.86/src/goGenerateTests.ts, you will see that there are 3 functions generateTestCurrentPackage, generateTestCurrentFile and generateTestCurrentFunction corresponding to the 3 commands. Each does a slightly different operation and passes the information to the common function generateTests

The need to fetch the flags from the vscode configuration is internal to generateTests. Since it needs a uri to fetch the configuration, the 3 functions generateTestCurrentPackage, generateTestCurrentFile and generateTestCurrentFunction can send the uri instead of the path.

• The function for package can call generateTests({dir: vscode.Uri.file(dir)});
• The function for function can call generateTests({dir: editor.document.uri});
• The function for cursor can call generateTests({dir: editor.document.uri, , func: funcName});

The generateTests method can then use the uri to fetch the configuration and uri.fsPath to get the path to pass to gotests

[jeffbean]

Ah ok. I see now. I will update this. Thanks for the time and patience reviewing!

[jeffbean]

Friendly ping on this thread. The feedback has been incorporated.

[ramya-rao-a]

@jeffbean So sorry, somehow this got lost in the sea of notifications.

Passing the uri from goMain will error out when the commands are run in the absence of active text editor.

I have pushed a commit to fix that, please take a look and try out the new flags

[jeffbean]

Awesome thanks for catching this. I tested your changes locally and it works as expected.

[ramya-rao-a]

@jeffbean I pushed another commit for the case when the flags in the configuration are any of the ones we add anyway.
Also updated the name of the setting, can you take another look?

[jeffbean]

Works great. Thanks again for all the help!

I pushed a lint fix to get the build back to passing:

src/goGenerateTests.ts[133, 1]: trailing whitespace
src/goGenerateTests.ts[136, 30]: == should be ===

[ramya-rao-a]

Thanks!