Linter integration on PRs

[ajnavarro]

To avoid comments on PRs related to formatting issues or imports order, we should use linters integrated on PRs. As a proposal we can use:

• Go: https://www.codefactor.io and https://golangci.com/
• Python: https://www.codefactor.io and most used linter on ML team (maybe @vmarkovtsev can give some insights here)
• Scala: https://www.codefactor.io + scalacheck on CI over SBT.

[vmarkovtsev]

We are using flake8 with some deep customization for Python.

[smola]

There was some past discussion about enabling golint globally too: src-d/ci#101

[smola]

@ajnavarro What does codefactor check? I have tried it in a couple of Go projects and it did not report issues.

golangci seems to run a collection of the usual Go linters, I'm afraid it might be too verbose? https://github.com/golangci/golangci-lint

[ajnavarro]

@smola here is an example on gitbase: https://www.codefactor.io/repository/github/src-d/gitbase/issues/master

[ajnavarro]

we already have golangci on gitbase too and it is not verbose

[se7entyse7en]

@src-d/applications don't use anything except for editors running goimports on save.

[dpordomingo]

About import orders, as mentioned in the issue desc, are we still keeping this old consensus?

when we group imports in Go (in blocks of standard + internal + 3rd party), are external packages by us considered internal or 3rd party?; example: in src-d/core, do we group src-d/framework imports together with src-d/core or do we treat them as 3rd party? I've been considering them as 3rd party [...]
(@smola from slack)

for me, internal means “very same repo"
(Sergio Arbeo)

If so, goimports just keep each block ordered, no matter what's inside, so it does not enforce any other convention we can have as a team.

[smacker]

don't use anything except for editors running goimports on save.

oh, sorry. I thought the question was only about code format. I (and most probably everybody else on the team) run go-lint which is enabled by default in VS Code.

[creachadair]

In general I support having some standard lint/check tools for PRs. But most of our code does not use them (or uses them inconsistently), and it can be really noisy to impose a standard after the fact. If we want to choose standard tools, I think we should also plan time to go through all our code and make it lint-compliant in PRs that do not contain other work. Otherwise it's really noisy everytime you touch some piece of code that hasn't been updated yet.

(For Go, at least, this will not be too hard; it is more tedious for some other languages where the warnings may require more changes to fix)

[creachadair]

I would strongly prefer we use tools that can be run from the command line, and do not require integrating with some external website. So things like golint and staticcheck are good candidates for Go. I haven't actually used CodeFactor, but I wasn't able to find a CLI for it—do you know if that exists?

[lwsanty]

@creachadair what do you think about golangci-lint ?

[creachadair]

@creachadair what do you think about golangci-lint ?

That looks pretty good. It has stable install points for use in CI (makes sense, given its heritage) and appears to support override configuration. It's pretty noisy in its default configuration, even on a project that is clean for golint and staticcheck, but the CLI looks reasonable.

[lwsanty]

@creachadair @smola
I did a PR to CI with ability to run linter using make command, it supports both config file and non-config mode with a couple of options to lower the noise. Hope it could be useful for someone.
src-d/ci#114