-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
🌱 golangci-lint: add recommended revive checks to linter-settings but d… #3034
🌱 golangci-lint: add recommended revive checks to linter-settings but d… #3034
Conversation
oscr
commented
Oct 22, 2022
- Updates golangci-lint version from 1.49.0 -> 1.50.1
- Adds all recommended revive checks to the config file. However to keep this pr simpler some new checks where disabled. But at least it's explicit now there are recommended checks that aren't enabled. I would gladly look into each of the disabled checks in follow up prs.
- Fixes the finding:
…isable checks with to many findings. Fix findings
@@ -13,8 +17,42 @@ linters-settings: | |||
enable=fieldalignment: true | |||
revive: | |||
rules: | |||
- name: if-return | |||
disabled: true | |||
# The following rules are recommended https://github.com/mgechev/revive#recommended-configuration |
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 think we merged the revive by mistake.
Why should we have revive and golangci?
Also, note that we are only using golangci and we do not test the projects with revive.
In this way, ihmo we need to remove:
linters-settings:
govet:
enable=fieldalignment: true
revive:
rules:
- name: if-return
disabled: true
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.
To be honest I just noticed revive was enabled but not with the recommended configuration and therefor I made a pr to work towards the recommended configuration. I believe that if you remove that section you will get default configuration instead.
As for the question of should there be / how much linting should a project have. I think that's a question you have more experience to answer. IMO there is a value for some linting. The question is always where do we get most value out of the least amount of linting overhead.
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.
HI @oscr,
I believe that if you remove that section you will get default configuration instead.
I would we get that?
We are not downloading or testing using revive. We just use golangci.
Am I missing something here?
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.
@camilamacedo86 golangci-lint is not a linter in itself, it's a linter runner. So you can configure golangci to run fewer or more or less linters. Then to make everything more complicated some linters (such as revive) are metalinters
and they can be configured with even more checks.
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.
Hun, I catcher. So, we are calling the revive checks when you run make lint
and test the project with golangci.
In this way, I am ok with the changes made too.
- name: indent-error-flow | ||
- name: errorf | ||
- name: empty-block | ||
disabled: true # TODO: Investigate if it should be enabled. Disabled for now due to many findings. |
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.
If that is the recommendation we might want in a follow up fix the issues and then enable right?
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.
Yes, I think that would be good to do. You might want them. You might not. But I disabled the checks that found many / non trivial findings to keep the complexity and size of this pr smaller.
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 am OK with the changes made.
Would be nice to fix the lint issues found in the other checks in follow-ups 🥇
@oscr see that I updated the PR title to use 🌱 . This change does not affect end-users and the PR should not be used to create the changelog. It only changes the checks used to lint the KB code itself.
/ok-to-test
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86, oscr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |