Concept of Elastic linter

[ewgRa]

During #5954 review @ruflin ask about changes like: please remove new line here, please use logp.Err instead of logp.Warn.

Looks like there is some coding style rules in Elastica, that can be introduced as a CI build process and make no sense to spend human time on this things.

I prepare https://github.com/elastic/beats/pull/6272 pull request as concept of linter, that can output for example something like that: "test.go:20: Must be no newline before check error" for code like that: \n\nif err == nil ...

Can you please make a quick look and maybe say something about concept? How you see it? Do you need it? Just to be know that I'm in right direction and it make sense to continue working on it.

From my side, I have something to discuss.

Is it linter, or formatter? There is good start https://github.com/golang/lint, "Golint differs from gofmt".

Must it check all files in build? Or just changed in this CI build? I think with checking all files there will be a problem that a lot of them already incompatible with some coding style rules.

Hope to hear feedback. Thanks

[andrewkroh]

I would rather not take on the task of maintaining a linter for Go as part of this project. Perhaps it could be in it's own repo and this project uses it just like it does goimports.

My biggest concern about adding a tool like this to the build is false positives. Personally I'm pretty happy with the results we get from running golint and goimports on PRs (automated by Hound CI and Travis/Jenkins, respectively). But if we could define a set of supplemental linter rules for Go that do not produce a lot false positives then this could be helpful in reviews. I often run gometalinter on PRs.

My guess is that writing a linter is easier because you do not need to reformat the code. This gives some leeway w.r.t to false positives since users can just ignore the warning.

Must it check all files in build? Or just changed in this CI build? I think with checking all files there will be a problem that a lot of them already incompatible with some coding style rules.

The tool itself should check the files as directed on the CLI (e.g. mylinter ./...). There are other tools that can handle producing warnings only for the changed files. We do this for golint's output when make lint is executed (see our Makefile). It uses reviewdog to do this.

[ewgRa]

Perhaps it could be in it's own repo and this project uses it just like it does goimports.

I thought about that, just fear that some checks can be super custom and depends on Beats code, since I knew only about two of them. This two can be definitely implemented in own repo. Ok, if I have the same thoughts, than probably it must be another repo.

My biggest concern about adding a tool like this to the build is false positives

It is very depends on the rules and exceptions from this rules. It can be more strict and miss some real cases, than generate false-positive.

My guess is that writing a linter is easier because you do not need to reformat the code

Some rules probably will be hard to fix, that's true.

Ok, I see now. It must be another repository. It must be linter and a fixer. You can lint and you can fix if you wish. There will be two type of checks: strict and not strict (maybe fixable, not fixable). Not strict checks can't be fixed, to avoid false-positive fixes. This mean that if you have set of strict and not strict checks, after run fix - linter still will be report something about not strict changes.

In this case projects will operate with this types of checks and kind of projects will decide, is it linter, or linter-fixer, or mix. Fail on build, or just use as helper and so on.

Looks good and can work. What do you think?

[ewgRa]

Ok, lets try with this one #6496, right now works as recommender. If it will found something for coding style - it will create a comment in pull request.

What do you think?

[ewgRa]

I try run it on all files, there is probably some false-positives, like

if err := p.init(results, &config); err != nil {

that we can probably exclude from this checker, but also good catches, like:

File ./libbeat/processors/actions/decode_json_fields.go
    recommendation at line 103: No newline before check error
    recommendation at line 169: No newline before check error

[leehinman]

@ewgRa Thank you for all the hard work on the project but getting a linter integrated hasn't been a top priority. If we decide to add a linter in the future we will definitely look at gocsfixer.