Skip to content
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

Fine-tune the Revive linter #3763

Merged
merged 2 commits into from
Dec 28, 2022
Merged

Fine-tune the Revive linter #3763

merged 2 commits into from
Dec 28, 2022

Conversation

MichaelSnowden
Copy link
Contributor

@MichaelSnowden MichaelSnowden commented Dec 24, 2022

What changed?
I turned on every linter for Revive except a few handpicked ones that I thought weren't useful.

Why?
We don't want to enable some useless linters like exported, but having linters like cyclomatic for new code seem really useful.

How did you test it?
I ran it locally. I also inspected the actual errors generated using this JQ query to see if they made sense:

golangci-lint run  --verbose --out-format=json --max-issues-per-linter=0 --max-same-issues 1 \
  | jq '.Issues | map((.Text| split(": ") | {"Rule": .[0], "Value": .[1]}) + {"Key": "./\(.Pos.Filename):\(.Pos.Line)"}) | group_by(.Rule) | map({"Rule": .[0].Rule, Violations: ([.[]] | from_entries )}) | sort_by(.Violations | length)'

Potential risks
It's just a linter

Is hotfix candidate?
No

@MichaelSnowden MichaelSnowden requested a review from a team as a code owner December 24, 2022 00:41
revive:
severity: error
confidence: 0.8
rules:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible not to include enabled rules? If new rules are added later I would like them to be enabled by default and only some of them explicitly disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is. Fixed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix PR description also :-)

@MichaelSnowden MichaelSnowden force-pushed the snowden/revive branch 2 times, most recently from 9d630e8 to f95af34 Compare December 26, 2022 14:37
@MichaelSnowden MichaelSnowden enabled auto-merge (squash) December 28, 2022 17:34
@MichaelSnowden MichaelSnowden merged commit d970aa4 into master Dec 28, 2022
@MichaelSnowden MichaelSnowden deleted the snowden/revive branch December 28, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants