-
Notifications
You must be signed in to change notification settings - Fork 74
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
Introduce go linting to CI process #65
Comments
As part of this we will have to triage/fix what is bad in the existing codebase as well since we will want the builds to fail if the linting tool comes back with any issues. |
IMHO, the minimum responsible configuration for go linting is
For reference, in case anyone cares, this is the standard config I use on my projects, which only reflects my personal opinion on what matters, and should in no way be considered a universal ideal :) [output]
format = "colored-line-number"
[linters]
enable = [
"interfacer", "gocyclo", "unconvert", "goimports", "unused", "varcheck",
"vetshadow", "misspell", "nakedret", "errcheck", "golint", "ineffassign",
"deadcode", "goconst", "vet", "unparam", "gofmt"
] |
@flimzy thanks for sharing. After looking through the list of linters/tools available that looks good to me. Two things I think I would add or want to do.
|
Both good points. I built my list with an old version of the tool. I expect there are new defaults since then, and I should probably add them to my own config. And for the |
A slight tangent, I wonder if it would be worth adding nancy to golangci-lint, using their custom linter instructions. |
Re: adding Nancy to golangci-lint.... I’m not sure it feels like the right place to add it. I’d love to add it in to potentially get more usage and keep more security issues out of production but really that tool of tools is more geared towards code level issues and Nancy is more of a dependency level tool. |
A valid point. Although there is one other dep-related linter in golangci-lint already:
Anyway, it's not really my place to say what should be done here. The thought just crossed my mind, so I thought to pass it along. |
The issue that spurred PR #59 was found via running a go linter. https://github.com/golangci/golangci-lint to be specific. But we should get this or at a dead minimum
go vet
in place as part of our CI pipeline. This will help our code suck less.What feature or behavior is this required for?
Doing linting on all PRs and master builds
How could we solve this issue? (Not knowing is okay!)
Introduce
golangci-lint
orgo vet
as part of the build process.Anything else?
golangci-lint
has a lot of configurability as well as lots more linters/tools that come with that have the ability to help find and fix issues. Whereasgo vet
is a do one thing and only that thing. It did point out the same issue fixed as part of Avoid copying a lock by value, which is invalid. #59 but that was it.golangci-lint
does in fact rungo vet
as part of its process.The defaults for
golangci-lint
are as follows :@flimzy mentioned that he likes to configure some others that are not enabled by default so hopefully they will fill us in on those details and we can discuss further.
I currently dont have strong opinions on what we enable at the moment. At this point getting either in place or even a different option is better than the nothing we have today.
cc @bhamail / @DarthHater / @flimzy
The text was updated successfully, but these errors were encountered: