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
Add config for golangci #943
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
run: | ||
deadline: 5m | ||
|
||
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 | ||
|
||
|
||
linters-settings: | ||
govet: | ||
check-shadowing: true | ||
goling: | ||
mstoykov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
min-confidence: 0 | ||
gocyclo: | ||
min-complexity: 25 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
I guess having at at 30 won't help all that much... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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+ ... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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 |
There was a problem hiding this comment.
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 issuesThere was a problem hiding this comment.
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 PRThere was a problem hiding this comment.
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...