-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
[MM-57743] Enable errcheck linter #26723
Conversation
|
|
|
f001f8f
to
81e8497
Compare
81e8497
to
f50d151
Compare
skip-dirs: | ||
exclude-dirs: |
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.
skip-dirs
doesn't exist. Fixing that as part of this PR.
@@ -26,11 +26,10 @@ linters: | |||
- goimports | |||
- makezero | |||
- whitespace | |||
# TODO: enable this later |
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.
4 years later, we can finally remove this
// FixInvalidLocales checks and corrects the given config for invalid locale-related settings. | ||
// | ||
// Ideally, this function would be completely internal, but it's currently exposed to allow the cli | ||
// to test the config change before allowing the save. | ||
func FixInvalidLocales(cfg *model.Config) bool { | ||
// fixInvalidLocales checks and corrects the given config for invalid locale-related settings. | ||
func fixInvalidLocales(cfg *model.Config) bool { |
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.
Fixing the comment while touching the package
// utils.TranslationsPreInit errors when TestFixInvalidLocales is run as part of testing the package, | ||
// but doesn't error when the test is run individually. | ||
_ = utils.TranslationsPreInit() |
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 haven't found out why, though.
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.
Do we know the error? Maybe we can create a ticket to track or create a HW issue.
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.
LGTM
// utils.TranslationsPreInit errors when TestFixInvalidLocales is run as part of testing the package, | ||
// but doesn't error when the test is run individually. | ||
_ = utils.TranslationsPreInit() |
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.
Do we know the error? Maybe we can create a ticket to track or create a HW issue.
Summary
The PR enables the
errcheck
linter, something that was planned for a few years now.The ignore list is quite broad. We can scope it down more granularly once we have a clear idea of how we do that. (Campain, staff member, don't work on it for now?)
First, I want to fix the issues in the enterprise code, as we can't do that with the community.
Ticket Link
https://mattermost.atlassian.net/browse/MM-57743
Release Note