tools: Which linters should we add

[ValarDragon]

There are several linters we can use. Please comment with your thoughts on using the following linters! There are more linters here, if you think that we should run them on circle ci: https://github.com/alecthomas/gometalinter

• unparam - (#1084) checks for unused function parameters on unexported functions
• go vet - #1370 catches bugs that can otherwise compile. (With the compound struct thing disabled)
• errcheck - makes sure that we check all errors
• ineffassign - Detect when assignments to existing variables are not used.
• varcheck - look for unused variables
• unconvert - Look for unneccessary type conversions

If we agree with a linter in most places, but not in a couple, we should still use it, and just add nolint in the places we disagree.

[liamsi]

I agree that all mentioned linters should be included! Additionally, we could add: http://golangci.com/

Basically, it is gometalinter but comments directly on the lines of code. It would save us a bunch of time, digging through circleci output. Becomes more relevant the more external contributors we have (saves us time reviewing).

[ValarDragon]

Forgot to transcribe the comments from #1372.

cwgoes: Can we ensure that local output and CI output are the same and add a make lint target so that developers can check linter output without waiting for CI? valardragon: Will do. I'll add a make test_lint command to this PR, but I'd rather we decide which linters to use first before making the command.

cwgoes: That's fine - but ideally we avoid special-casing the CircleCI config; CircleCI should just run make get_tools lint.

jlandrews: I'm all for maximal metalinting +1 rigelrozanski: unparam is really cool - we don't pass unparam right now aka there are a few limp-dead variables

valardragon: I'm conflicted on whether or not deadcode is useful. Here is its output:

It gave a couple of useful points, e.g. baseapp/baseapp.go:26:1:warning: dbHeaderKey is unused (deadcode), means we should either remove that or write a test case to cover that. But the majority of the output is for unused vars in test helper files, which I don't think is code that we need to change. We'd have to add a bunch of nolint statements to the test files though. I'm leaning towards removing deadcode.

valardragon: Looks like structcheck doesn't do what we want. It only checks at the module level, not at the whole project level. This may be something that can be fixed though. I'm going to remove it from here for now, until a better tool / structcheck has been forked. I'll remove deadcode too, we can always add it back if we like its output.

valardragon: Note, I've disabled errcheck output on the client directories. This was because the errors there don't really contribute to failing fast / aren't that helpful. (i.e. not checking errors on writing the http responses)

[rigelrozanski]

status on unparam? can it be integrated into metalinter?

[ValarDragon]

It can, I just haven't fixed all of the unparam errors yet. (I'm planning on fixing them later today)

[ValarDragon]

varcheck isn't catching anything new, so don't really see a reason to add it. So thats it for linters I want to add! All thats left now is to see if everyone wants to use the linting thing that comments directly onto PR's which Ismael mentioned, or if everyone would rather us just have make test_lint

[rigelrozanski]

Hay! that tool Ismail mentioned sounds cool! We should do it (unless there is a reason not to).

I agree that it will be useful for external contributors

[rigelrozanski]

@ValarDragon can we close this issue do you think? - if there are any in that list which you think we should still be implementing but haven't yet, let's open new issues for them

[ValarDragon]

I think we just need to use the tool that ismail suggested, though we could open a new PR for that. (Figuring out integrations for that maybe a postlaunch matter though)

[rigelrozanski]

cool - if you have a chance let's issue swap to a new one, or rename this one appropriately with a new comment in the issue body