New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Concept of Elastic linter #6273
Comments
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.
The tool itself should check the files as directed on the CLI (e.g. |
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.
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.
Some rules probably will be hard to fix, that's true. Ok, I see now. 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? |
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? |
I try run it on all files, there is probably some false-positives, like
that we can probably exclude from this checker, but also good catches, like:
|
@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. |
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 #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
The text was updated successfully, but these errors were encountered: