Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Add basic multi-package mode #830

Closed
wants to merge 1 commit into from
Closed

Add basic multi-package mode #830

wants to merge 1 commit into from

Conversation

mattblagden
Copy link

Add an option to run lint/build/test on the entire workspace.

This is a very basic implementation that checks for the go.multiPackage flag when inspecting the code. If in multi-package mode:

  • The working directory is set to the workspace root instead of the current file's directory
  • Build options that aren't supported for multiple packages are omitted
  • The target is switched from . to ./...

See issue #589

@msftclas
Copy link

msftclas commented Mar 2, 2017

@mattblagden,
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

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • vet and cover on save features are broken
  • consider making the setting an object where each feature(build/lint/vet) can be set to true or false separately
  • consider skipping adding multipackage for test for now
  • All in all, very excited and happy to see this PR. Keep up the good work :)

@@ -213,8 +231,13 @@ export function check(filename: string, goConfig: vscode.WorkspaceConfiguration)

if (!!goConfig['vetOnSave']) {
let vetFlags = goConfig['vetFlags'] || [];
let args = ['tool', 'vet', ...vetFlags];
if (multiPackage) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vet is not working regardless of the value of go.multipackage
go tool vet needs either filepath, package name/path or directory path

@@ -484,6 +484,11 @@
"type": "boolean",
"default": false,
"description": "If false, the import statements will be excluded while using the Go to Symbol in File feature"
},
"go.multiPackage": {
"type": "boolean",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, this flag affects build, lint, vet and test. There might be cases when users might want just a few of these to run on whole workspace. Example: There might be a lot of tests in the workspace and they might take time, so user might not want test to run on whole workspace but only build.

So instead of just having this setting as a simple boolean, we can have it as an object

"go.multiPackage": {
          "type": "object",
          "default": {
            "build": false,
            "test": false,
            "lint": false,
            "vet": false
          },
          "description": "Invoke build/lint/test commands recursively from the workspace root."
        }

Thoughts?

"go.multiPackage": {
"type": "boolean",
"default": false,
"description": "Invoke build/lint/test commands recursively from the workspace root."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vet is missing in the description

}
}
if (multiPackage) {
args = args.concat('./...');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coverOnSave is broken when multiPackage is set to true
To begin with, we can skip test from the multiPackage feature, and continue working on it in a separate PR

#816 talks about using test results from multiple packages for getting code coverage. We can use that to track multiPackage feature for test and code coverage

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes as per previous comments

@coccyx
Copy link

coccyx commented Jun 4, 2017

Would love to see this go in. Looks like some work is remaining, but +1 from me for this feature.

@ramya-rao-a
Copy link
Contributor

Closing this in favor of #1023

@ramya-rao-a ramya-rao-a closed this Jun 9, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants