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

Update installTools logic to support optional tools #509

Merged
merged 5 commits into from
Oct 6, 2016

Conversation

ramya-rao-a
Copy link
Contributor

@ramya-rao-a ramya-rao-a commented Oct 2, 2016

The previous fix to #467 added goimports to the list of tools to be installed.
For users who have not chosen goimports as their formatting tool, this change will prompt them to install go imports via the Analysis Tools Missing status bar item.

The current PR makes it so that user will be prompted to install goimports only if it has been chosen as their formatting tool

golint has dropped support for Go 1.5 This would break installing of the golint tool on machines having Go 1.5 This also breaks travis

Ideally, golint should provide a tag/branch which works for Go 1.5 and we should be able to use that.
Until then, we suggest such users to use gometalinter instead

@ramya-rao-a ramya-rao-a changed the title prompt to install goimports only if it is set as the formatting tool by the user Update installTools logic to support optional tools Oct 5, 2016
};

// Install the formattool that was chosen by the user
if (goConfig['formatTool'] === 'goimports') {
Copy link
Member

Choose a reason for hiding this comment

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

Is gofmt included with Go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes


export function installAllTools() {
installTools(Object.keys(tools));
getGoVersion().then(() => installTools(Object.keys(getTools())));
Copy link
Member

Choose a reason for hiding this comment

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

getTools is called here and then again multiple times in installTools

Copy link
Member

Choose a reason for hiding this comment

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

Also you're not using the promise in then, why not do this?

getGoVersion().then(version => installTools(Object.keys(getTools()), version));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The version is used internally by getTools() and not installTools directly.

Refactored installedTools to not repeatedly call getTools() for each missing tool

@@ -1,7 +1,6 @@
language: go

go:
- 1.5
Copy link
Member

Choose a reason for hiding this comment

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

Can we amend the tests to not lint on 1.5 instead?

Copy link
Contributor Author

@ramya-rao-a ramya-rao-a Oct 5, 2016

Choose a reason for hiding this comment

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

Its not the tests that fail, its the install step where we install golint.
go get -u -v github.com/golang/lint/golint

I could write a custom install script that looks at the go version and then takes the decision whether or not to install golint
But, thats for another day.
I am hoping that golint creates a tag/branch that works for Go 1.5, then I can put 1.5 back here in travis, add a custom install script to target that tag/branch

Copy link
Member

Choose a reason for hiding this comment

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

I'm concerned about removing the 1.5 test and then never adding it back again. I think it's worth adding a conditional to https://github.com/Microsoft/vscode-go/blob/master/.travis.yml if that's all that needs to happen.

Copy link
Member

Choose a reason for hiding this comment

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

Does this work?

if [ "$(go --version)" != "1.5" ]; then go get -u -v github.com/golang/lint/golint; fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

am trying this

if [[ "$(go version)" =~ "go version go1.5" ]]; then echo cannot get golint; else go get -u -v github.com/golang/lint/golint; fi

@ramya-rao-a
Copy link
Contributor Author

@Tyriar All comments have been addressed, I think we are good to go

@ramya-rao-a ramya-rao-a merged commit 91637fb into microsoft:master Oct 6, 2016
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.

3 participants