Add config for golangci #943
Conversation
currently most checks are enabled but we are only fixing error going forward and all current problems will stay and will be fixed as we refactor the code around them.
Codecov Report
@@ Coverage Diff @@
## master #943 +/- ##
=======================================
Coverage 70.35% 70.35%
=======================================
Files 118 118
Lines 9111 9111
=======================================
Hits 6410 6410
Misses 2294 2294
Partials 407 407Continue to review full report at Codecov.
|
| max-issues-per-linter: 0 | ||
| # Maximum count of issues with the same text. Set to 0 to disable. Default is 3. | ||
| max-same-issues: 0 | ||
|
|
There was a problem hiding this comment.
Not sure how golangci will work when this is merged in master, but maybe we also need new: true here, so we're not buried by all of the old issues
There was a problem hiding this comment.
as far as I understand new means only for what is currently not committed or if there is nothing the last commit. Which is not what we want - we want when this is a PR for it to look at what it will be merge and check against it which .. maybe golangci.com does ? maybe it just runs with --new by default ... Maybe they can write it somewhere on their page ... Given that this is how everybody uses it and at least in some of them they have issues in master but master is still green I would guess golangci does the right thing™
Looking at https://golangci.com/r/github.com/loadimpact/k6/pulls/943 they make a patch and then analyse that ... they also git clone with depth 1 so ... maybe they are doing what --new will do by default ... I will need to find a way to test this ... probably with a bogus PR
There was a problem hiding this comment.
Yeah, I guess we can just wait and see what happens when we merge this (or another) PR with master. Worst case, we have a "broken" commit, not the end of the world...
| goling: | ||
| min-confidence: 0 | ||
| gocyclo: | ||
| min-complexity: 25 |
There was a problem hiding this comment.
25 may be a bit low, considering that 30 is the default. I'm all for splitting apart huge methods, but I feel like this setting will be problematic if it complains when we want to make a change in one of the currently overly "complex" methods:
golangci-lint run --out-format=tab --disable-all --enable gocyclo --max-issues-per-linter 0 --max-same-issues 0 ./... :(
js/modules/k6/html/elements_test.go:87:1 gocyclo cyclomatic complexity 112 of func `TestElements` is high (> 25)
js/modules/k6/html/html_test.go:65:1 gocyclo cyclomatic complexity 97 of func `TestParseHTML` is high (> 25)
js/modules/k6/http/request.go:172:1 gocyclo cyclomatic complexity 64 of func `(*HTTP).parseRequest` is high (> 25)
converter/har/converter.go:61:1 gocyclo cyclomatic complexity 61 of func `Convert` is high (> 25)
js/modules/k6/html/element_test.go:56:1 gocyclo cyclomatic complexity 50 of func `TestElement` is high (> 25)
js/modules/k6/http/request.go:402:1 gocyclo cyclomatic complexity 50 of func `(*HTTP).request` is high (> 25)
js/modules/k6/ws/ws.go:82:1 gocyclo cyclomatic complexity 44 of func `(*WS).Connect` is high (> 25)
core/local/local.go:152:1 gocyclo cyclomatic complexity 40 of func `(*Executor).Run` is high (> 25)
js/common/bridge_test.go:291:1 gocyclo cyclomatic complexity 40 of func `TestBind` is high (> 25)
lib/options.go:302:1 gocyclo cyclomatic complexity 37 of func `(Options).Apply` is high (> 25)
js/common/bridge.go:116:1 gocyclo cyclomatic complexity 32 of func `Bind` is high (> 25)
js/bundle_test.go:48:1 gocyclo cyclomatic complexity 28 of func `TestNewBundle` is high (> 25)
I guess having at at 30 won't help all that much...
There was a problem hiding this comment.
yeah ... I dunno I suppose leaving it at 25 is fine for as I am not getting all the way up to 50+ ... //nolint will be the tool for the job it seems :(
I will probably need to merge this in #907 in order to //nolint some the method in js/modules/k6/http/request.go
There was a problem hiding this comment.
Yeah, I'm OK to leave it at 25... Rising it to 50 would be absurd, and even that won't be enough. And from what I remember, most of those errors above are actually problematic functions that we're slowly working on splitting up and refactoring... My only issue with needing to add //nolint:gocyclo to them for a small change will be that we might forget to eventually remove those //nolint statements... We'll see how it goes I guess, in any case, it will still be better than the current situation 😄
.golangci.yml
Outdated
| run: | ||
| deadline: 5m | ||
| skip-files: | ||
| - ".*gen_elements.*\\.go$" |
There was a problem hiding this comment.
This is wrong. gen_elements.go is the hand-written file and elements_gen_test.go is also a hand-written test file. The only generated file is elements_gen.go 😄 And even then, I don't think we should exclude any of these files from the linter.
The only files we should exclude are the rice-box ones, since they're just static embedded content. The rest are actual Go code, and it doesn't matter if they're generated or not, they shouldn't have linter issues unless there's a very good reason. And for those with good reasons, //nolint should suffice
There was a problem hiding this comment.
adding //nolint to a generated file will be hard ... especially if we are not the ones generating it which is not the case. On the other hand golangci-lint automatically removes all autogenerated files it detects ... which obviously doesn't work in this case :)
currently most checks are enabled but we are only fixing error going
forward and all current problems will stay and will be fixed as we
refactor the code around them.