Run on-save tools for entire workspace by vapourismo · Pull Request #1023 · microsoft/vscode-go

vapourismo · 2017-06-07T15:55:19Z

Changes the default behavior from running lint/vet/build for the package of the current active file to running it for the entire workspace.

Potential downside is not having a flag which allows the behavior to be changed between current package and workspace (as with #830). Something for the future.

msftclas · 2017-06-07T15:55:27Z

@vapourismo,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

ramya-rao-a · 2017-06-07T19:27:16Z

We do need a flag to control current package vs whole workspace.

In #830 we talked about a setting go.multiPackage, but thinking more on this, something like the below would be better instead of adding another setting.

 "go.buildOnSave": {
          "type":"string",
          "enum": [            
            "currentPackage",
            "currentWorkspace"
          ],
          "default": "currentPackage",
          "description": "Run 'go build'/'go test -c' on save on either the current package or the current workspace."
        }

With this, people who have set go.buildOnSave to true or false beforehand, will get a linting error

In the code, we can continue to honor either of true, false as well as the new values so that we are backward compatible.

ramya-rao-a

Please add the setting to control package vs workspace

vapourismo · 2017-06-07T21:14:42Z

Which value should you set, if you want it to be disabled? null or "nothing"?

ramya-rao-a · 2017-06-07T21:48:25Z

How about off ?

vapourismo · 2017-06-07T22:11:12Z

Okay, I added these options as workspace, package and off.

~~There are those two failed tests in the CI which only occur for macOS. Seems to be something with gometalinter. They seem odd to me, since I didn't modify how the linter is invoked.~~

vapourismo · 2017-06-07T22:13:40Z

Nevermind, it is just the tip line that is failing. And it seems unrelated to this PR.

ramya-rao-a · 2017-06-07T23:36:30Z

src/goCheck.ts

+
+		if (buildWorkspace) {
+			buildWorkDir = vscode.workspace.rootPath;
+			args.push('./...');


When the current file that is open is a test file, this will end up running go test -i ./... which will run the tests in the workspace instead of trying to compile them

For compiling test files, we need to use go test -c and -c cannot be used on multi-package.

@rakyll @campoy @mattetti Do you know of a way to compile test files under a directory?

vapourismo · 2017-06-08T10:54:21Z

How about this? Original behavior with package should be the same, I think.

ramya-rao-a · 2017-06-08T17:56:25Z

src/goCheck.ts

+ * This ignores hidden directories and their sub-directories.
+ * @param dirPath Directory in which to start searching
+ */
+function findTestDirectories(dirPath: string): string[] {


I appreciate the work here. Consider this

User with a very large workspace with deeply nested directories

User with auto-save on

The directories check here is synchronous.
We later wait for all promises (build, lint, vet) to finish before showing diagnostics to the user.

So you can imagine for each save, this might delay results more than needed.
Note: We have #1015 tracking to find a way to optimize this.

Also, I'd much rather prefer a better way to compile test files from Go itself.

Until then, we can skip trying to compile the test files in the whole workspace.

if (buildOnSave is not 'off') { if (buildWorkspace) `go build -i -o ./...` if (current file is test file) `go test -i -c -o <import path>` else if (!buildWorkspace) `go build -i -o <import-path>` }

I've had the exact same thought, but I've come to the conclusion that it doesn't matter. If you set it to build the entire workspace, you should expect that to happen. In case somebody uses a big workspace, they can set it to build only the current package in the workspace-local settings.

Additionally, even though the directory traversal uses synchronous functions, it is way faster than the lint/vet/etc jobs. It appears that the extension accumulates the results of all "check"-Promises anyway, so delaying some for a couple milliseconds doesn't really hurt.

I understand and agree that this is not a particularly smart solution, but it should work for most use cases.

Also, I'd much rather prefer a better way to compile test files from Go itself.

Not sure, if that is going to happen. The only smart alternative is to run go test -run ! ./..., which will check the test files, but won't run any since they don't match the pattern. Could you work with that?

go test -run ! ./...

Lets give this a try. I was initially not in favor of this as the output is different (lots of "no test files" followed by a "build failed" on the pkg which has error) and we will have to write a different regex to do the problem matching.

But taking a closer look, there is also a line in there which matches our expectation and our current regex should work.

vapourismo · 2017-06-08T20:09:45Z

go test -run ! ./.. works, sort of. It also checks the packages, so there are duplicates from go build.

Interestingly, it also checks packages that do not contain tests. So theoretically go build and go test can be merged, in the check function.

Sets the current working directory to the workspace root when linting and vetting. Also appends the './...' argument which causes the tools to traverse the workspace recursively. Other tools are not affected. Additionally, this will utilize 'go vet' instead of 'go tool vet'.

Sets working directory for 'go build' command to workspace root. Also adds './...', so the entire workspace is traversed.

This modifies settings 'buildOnSave', 'lintOnSave' and 'vetOnSave'. The new options are 'workspace', 'package' or 'off' which will cause the respective on-save tools to run in the determined scope. Old values will still work as they did before.

'buildOnSave' compiles the test for the current package if the saved file is a test unit, no matter if it is set to 'workspace' or 'package'.

If 'buildOnSave' is set to 'workspace', the check function will traverse the workspace in order to find sub-packages that can be tested and subsequently compiles those tests.

Utilize 'go test' to check all tests in the workspace without actually running them.

Utilize 'go test -run=^$ ./...' when building entire workspace, 'go build' for current package and 'go test -c' for the current package when the current file is a test file.

vapourismo · 2017-06-08T20:27:22Z

Latest commit prevents duplicates.

ramya-rao-a

Thanks for all the work!

msftclas added the cla-already-signed label Jun 7, 2017

ramya-rao-a suggested changes Jun 7, 2017

View reviewed changes

ramya-rao-a suggested changes Jun 8, 2017

View reviewed changes

vapourismo added 7 commits June 8, 2017 22:24

Run build (on save) on entire workspace

326ce7a

Sets working directory for 'go build' command to workspace root. Also adds './...', so the entire workspace is traversed.

Build on-save shall not trigger test of entire workspace

4af8f6d

'buildOnSave' compiles the test for the current package if the saved file is a test unit, no matter if it is set to 'workspace' or 'package'.

Compile tests for entire workspace if settingss require

21a49b1

If 'buildOnSave' is set to 'workspace', the check function will traverse the workspace in order to find sub-packages that can be tested and subsequently compiles those tests.

Check tests for entire workspace when settings require it

70d5f72

Utilize 'go test' to check all tests in the workspace without actually running them.

Prevent running both 'go build' and 'go test' on save

78e9283

Utilize 'go test -run=^$ ./...' when building entire workspace, 'go build' for current package and 'go test -c' for the current package when the current file is a test file.

ramya-rao-a approved these changes Jun 8, 2017

View reviewed changes

ramya-rao-a merged commit ff47fea into microsoft:master Jun 8, 2017

ramya-rao-a mentioned this pull request Jun 9, 2017

Add basic multi-package mode #830

Closed

seanpfeifer mentioned this pull request Jul 10, 2017

"go.vetFlags" fails to vet code in 0.6.62 #1073

Closed

ramya-rao-a mentioned this pull request Jul 10, 2017

Refactor build workspace on save feature #1074

Merged

Conversation

vapourismo commented Jun 7, 2017

Uh oh!

msftclas commented Jun 7, 2017

Uh oh!

ramya-rao-a commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramya-rao-a left a comment

Choose a reason for hiding this comment

Uh oh!

vapourismo commented Jun 7, 2017

Uh oh!

ramya-rao-a commented Jun 7, 2017

Uh oh!

vapourismo commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vapourismo commented Jun 7, 2017

Uh oh!

ramya-rao-a Jun 7, 2017

Choose a reason for hiding this comment

Uh oh!

vapourismo commented Jun 8, 2017

Uh oh!

ramya-rao-a Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vapourismo Jun 8, 2017

Choose a reason for hiding this comment

Uh oh!

ramya-rao-a Jun 8, 2017

Choose a reason for hiding this comment

Uh oh!

vapourismo commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vapourismo commented Jun 8, 2017

Uh oh!

ramya-rao-a left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ramya-rao-a commented Jun 7, 2017 •

edited

Loading

vapourismo commented Jun 7, 2017 •

edited

Loading

ramya-rao-a Jun 8, 2017 •

edited

Loading

vapourismo commented Jun 8, 2017 •

edited

Loading