search

src-d / guide

Aiming to be a fully transparent company. All information about source{d} and what it's like to work here.
Creative Commons Attribution Share Alike 4.0 International
294 stars 101 forks source link

Linter integration on PRs #384

Open ajnavarro opened 5 years ago

ajnavarro commented 5 years ago

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:

vmarkovtsev commented 5 years ago

We are using flake8 with some deep customization for Python.

smola commented 5 years ago

There was some past discussion about enabling golint globally too: https://github.com/src-d/ci/issues/101

smola commented 5 years ago

@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 commented 5 years ago

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

ajnavarro commented 5 years ago

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

se7entyse7en commented 5 years ago

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

dpordomingo commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@creachadair what do you think about golangci-lint ?

creachadair commented 5 years ago

@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 commented 5 years ago

@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. https://github.com/src-d/ci/pull/114