Add style check

[zurk]

PML team start to use go features of CI.

But one for a style check is missing. Now we use this code to run it:

check-style:
	golint -set_exit_status ./...
	[[ -z $$(gofmt -s -d .) ]] && [[ -z $$(goimports -d . ) ]] || exit 1 \
		# Run `goimports -w . && gofmt -s -w .` to fix style errors
	go vet

I propose to include this code to the main CI file because it is general for any Go project.

@smola WDYT?

[smola]

@zurk we should make it conditional to the definition of a variable, e.g. CHECK_STYLE=true. Off by default. Otherwise, builds might start failing for other projects. Turning it on by default would be a change requiring a major version bump.

[creachadair]

As @smola says. And in particular, enforcing simplifications (-s) will often trigger failures for existing code that has not been using it. Some of the golint checks are also fairly aggressive, and golint doesn't support lint comments.

So if we do want to make this a standard (and that does seem reasonable), we should let each maintainer do a cleanup on their own schedule before we flip the default to ON.

[zurk]

Otherwise, builds might start failing for other projects.

I do not want to include this command to any existing command as a dependency, so only if you add make check-style somewhere in .travis.yml it will be triggered. So old projects are in safe. Here is an example how we do that:

https://github.com/src-d/eee-identity-matching/blob/a11c446d2562133e63523fcbccb8a16eaa8c56d2/.travis.yml#L44

So, maintainers have as many time as they want to start using this check.

I agree with @creachadair that this check should be a standard so we should think carefully what is worth to include.

What about gofmt check it is always easy to fix with something like

	gofmt -s -w .
	goimports -w .

I think it is worth to ask other golang guys what they think.

[creachadair]

Enforcing gofmt -s makes sense as a baseline. For my personal projects, I also enforce:

1. A run of go test -race -cpu=2 ./... must pass. This probably doesn't make sense as a style check, and I think a lot of our projects have peculiar test setups, so probably not a good fit here.

2. A run of go vet ./... must pass.

3. A run of golint -set_exit_status ./... must pass.

4. A run of staticcheck ./... must pass (link).

Of these, I feel like (2) and (3) probably make sense for us, and maybe (4) as an opt-in. (Once you get your code clean of warnings, it actually catches a lot of useful things—but it can be noisy if you've never run it before).