chore(lint): enable revive defaults and drop dead exclusions - #928
Merged
Conversation
Declaring linters.settings.revive.rules REPLACES revive's default set, so the 23 default rule names were re-listed ahead of the uber-go additions to avoid silently dropping enforcement. golangci-lint v2.12.2 accepts enable-default-rules, which restores those defaults without the re-declaration and removes the failure mode the re-declaration created: deleting one of the 23 lines loses a rule with no signal. Verified behavior-preserving rather than assumed: - golangci-lint v2.12.2's internal defaultRules list is a verbatim copy of revive v1.15.0's, and enable-default-rules feeds that same list through normalizeConfig, so the two spellings cannot diverge at this pin. - GL_DEBUG=revive dumps the resolved lint.Config. The dumps for the two spellings differ in exactly one field — EnableDefaultRules itself: same 40 rules, each with identical Arguments, Severity, Disabled and Exclude, plus Confidence 0.8 and Severity "warning". - A throwaway probe package planting a deliberate violation of each of the 23 defaults produced an identical finding set under both configs (22 rules fired; time-naming emits nothing under golangci-lint at this pin for any construct tried, and staticcheck ST1011 covers that class). - Repo-wide runs matched exactly, issue for issue: 0/0 with the real config, 166/166 with the exclusion presets stripped (88 revive: 68 exported + 20 package-comments), 0/0 with the two var-naming exclusion rules stripped. - No run logged level=error or "cannot find rule". Pre-existing drift noticed while checking the exclusions and left alone: the two `text: "var-naming: avoid package names"` exclusion rules match nothing at revive v1.15.0, because that package-name check moved out of var-naming into the (unenabled) package-naming rule and was reworded. The findings they suppressed are no longer produced at all, under either spelling.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughThe Revive configuration now enables default rules with ChangesRevive linting
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Both `text: "var-naming: avoid package names"` stanzas — for (cache|database|observability)/testing/ and trace/ — stopped matching at revive v1.15.0, when the package-name check moved out into a separate `package-naming` rule (not in the default set, not enabled here) and every message was reworded. That text exists only in v1.14.0. They were suppressing nothing: deleting them leaves the repo at 0 findings under `--uniq-by-line=false`, exactly as before. This is not a behavior change, it is removing config that had silently stopped applying. Found while proving the enable-default-rules swap in the previous commit: the exclusions match on message text, so they had to be re-verified rather than assumed, and re-verifying showed they were already inert. wiki/linting.md is updated for both commits. The revive section now presents enable-default-rules as what this repo does rather than as an alternative for new configs, and the exclusions section carries the transferable lesson: an exclusion keyed on message `text` dies silently when the linter rewords the message, in either direction, so prefer scoping by `path` and `linters`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gaborage
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wiki/linting.md`:
- Line 60: Update the wording in the linting documentation to use “scopes”
instead of “scopings” in the phrase describing the `_test.go` exceptions,
without changing the surrounding lint configuration references.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c0b97e38-e700-4a2d-a7a9-b37a1816168b
📒 Files selected for processing (2)
.golangci.ymlwiki/linting.md
💤 Files with no reviewable changes (1)
- .golangci.yml
"a few `_test.go` scopings" -> "a few rules scoped to `_test.go`". Not a word, and the rephrasing also names what is actually being described. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
wiki/linting.md (2)
65-68: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winScope the statement about suppressed findings.
The repository still contains 11 inline
var-namingsuppressions underdatabase/types/. Change “Nothing was being suppressed” to “The two exclusions suppressed nothing.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wiki/linting.md` around lines 65 - 68, Update the statement in the linting documentation to say that the two exclusions suppressed nothing, rather than claiming that nothing was being suppressed generally. Preserve the surrounding explanation about the two obsolete `var-naming` stanzas and their removal.
170-174: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winQualify the “strictly safer” claim.
When dependency versions change, recheck the resolved rule set. Replace “strictly safer” with “safer against accidental omissions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wiki/linting.md` around lines 170 - 174, Update the prose around the golangci-lint default-list flag to replace “strictly safer” with “safer against accidental omissions,” while preserving the surrounding explanation and caveat about incompatibility with enable-all-rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@wiki/linting.md`:
- Around line 65-68: Update the statement in the linting documentation to say
that the two exclusions suppressed nothing, rather than claiming that nothing
was being suppressed generally. Preserve the surrounding explanation about the
two obsolete `var-naming` stanzas and their removal.
- Around line 170-174: Update the prose around the golangci-lint default-list
flag to replace “strictly safer” with “safer against accidental omissions,”
while preserving the surrounding explanation and caveat about incompatibility
with enable-all-rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a88756b5-65eb-49e4-9594-83f1c9243c85
📒 Files selected for processing (1)
wiki/linting.md
Both were unqualified statements that read wider than the evidence behind them. "Nothing was being suppressed" sat in a paragraph advising readers to recheck their own exclusions, where it could be read as a claim about exclusions in general. Scoped to the two var-naming stanzas actually measured. "strictly safer" overstated enable-default-rules. It is safer against one specific failure — omitting a default when re-declaring by hand — and it costs the visible inventory that re-declaration gave. Now "safer against accidental omissions", which is the claim the evidence supports. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gaborage
enabled auto-merge (squash)
August 8, 2026 04:01
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
.golangci.ymlre-declared golangci-lint's 23 default revive rules, because declaringrulesreplaces the default set rather than extending it.enable-default-rules: truerestores them, so only the 17 uber-go additions need listing — and deleting one line can no
longer silently drop a rule. Also removes two
var-namingexclusions that stopped matchingat revive v1.15.0, and updates
wiki/linting.mdfor both.Impact
None — behavior-identical. In v2.12.2 the same
defaultRulesslice backs both the implicitdefault and
enable-default-rules, and it is a verbatim copy of revive v1.15.0's list.Consumers following
wiki/linting.mdnow get the flag rather than a 23-line transcription.Verification
Resolved-config dumps differ in exactly one field (
EnableDefaultRules), same 40 rules withidentical arguments. With exclusion presets stripped so the comparison is non-vacuous, the
full repo yields the same 166 issues (88 revive) under both, matched issue-for-issue. The
deleted exclusions suppressed nothing — removing them leaves the repo at 0 findings.
Summary by CodeRabbit
Chores
Documentation