-
Notifications
You must be signed in to change notification settings - Fork 372
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
chore(linter): update Golang linter configuration #1977
Conversation
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.
i appreciate your changes here. however, i think this PR is doing too much at once and should be broken into more singularly-focused pieces.
additionally your commit messages and PR descriptions should include details on why changes are being made and be more specific. for example "added a new make command", does not tell a reader of the git log what was actually changed. PR titles & commit messages should follow the guidelines outlined in https://www.conventionalcommits.org/en/v1.0.0/. Example: "feat(ci): enabled golang linters" (assuming this is the only thing the PR does, which is not the case for this PR).
regarding the linters: i think we need to first decide as a team what lints to use. i also think the final form of linting should prefer starting with the defaults and only adding or disabling functionality as the need arises, rather than starting with everything disabled and enabling individual linters/rules. this will also make the config file much simpler and easier to read. exceptions to the defaults, including additional new linters, should have explanatory comments in the config YAML.
some other thoughts/questions:
- please also delete the existing, incorrectly named config file
- please add @faddat as a co-author to the commit that changes the config file. he previously did a good amount of prep for the linters, including identifying the misnamed file in Linters haven't been run recently #1636 (thank you, @faddat!!)
- does the current codebase pass all these lints? are we sure the next commit to master won't fail CI because of things enabled here?
|
Co-authored-by: @faddat [email protected]
ada9d81
to
7f5df86
Compare
I agree that we should discuss and finalize the linters as a team. I can set up a time with the team to discuss this further. My approach was to add all linters initially, as I found that even the basic linters were failing, which made starting with a comprehensive setup more practical. The linters currently enabled and disabled in the configuration align with the existing "style" the team has developed. Starting with all linters that we can have enabled allows us to identify and address any issues early on. Once we have a baseline where everything is passing, we can then adjust the configuration by enabling or disabling specific rules as needed. This approach will help us ensure that we maintain a high standard of code quality from the start. I agree that the final configuration should be simplified and commented for clarity. Once we finalize the linters and rules, I'll make sure to document any exceptions and customizations in the config YAML. Let me know if you have any specific linters or rules you'd like to prioritize, and we can adjust accordingly. |
.golangci.yml
Outdated
for-loops: false # disables preallocation checks in for loops | ||
|
||
issues: | ||
exclude-rules: |
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.
can we remove the exclusions until they rear their ugly heads in future commits?
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.
looks great, thanks for the much needed addition!
* update golinter + add go sec * add golangci.yml Co-authored-by: @faddat [email protected] * update * update * fix release version * remove sec, update from pr comments, cleanup golangci.yml to not break on master * remove @faddat, not valid codeowner * remove unnecessary make command * remove incorrectly named golangci.yml file * add --new-from-rev * use master instead of main * remove extra echo * set the exports properly * add setup go to work with act * add some docs to golangci linter * test new-from-rev * enable more linters, but app.go back * verify issues-exit-code being gone * put it back * enable more linters * remove exclusions
Description
To test locally:
act -j golangci-lint
Strategy moving forward:
new-from-rev
that runs the linter against the files changed in a PR..golangci.yml
Checklist