Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add config for golangci #943

Merged
merged 4 commits into from Mar 8, 2019
Merged

Add config for golangci #943

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dfbaa0a
Add config for golangci
mstoykov Mar 8, 2019
c21182e
typo in golangci
mstoykov Mar 8, 2019
dc88089
skip generated files in golangci
mstoykov Mar 8, 2019
7527200
Remove some of the nongenerated files added by dc880890
mstoykov Mar 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 32 additions & 0 deletions .golangci.yml
View file
Open in desktop
@@ -0,0 +1,32 @@
run:
deadline: 5m
skip-files:
- "rice-box.go$"

issues:
# Maximum issues count per one linter. Set to 0 to disable. Default is 50.
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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...


linters-settings:
govet:
check-shadowing: true
golint:
min-confidence: 0
gocyclo:
min-complexity: 25
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

maligned:
suggest-new: true
dupl:
threshold: 100
goconst:
min-len: 2
min-occurrences: 2

na-- marked this conversation as resolved.
Show resolved Hide resolved
linters:
enable-all: true
disable:
- gochecknoinits
fast: false