Enforce RuboCop in CI, and clear the 895 offenses first - #190
Merged
Conversation
RuboCop has never been enforced here, so 895 offenses had built up across 75 files. 882 of them were Layout/SpaceInsideArrayLiteralBrackets: the repo uses rubocop-rails-omakase, which wants `[ a, b ]`, and much of the codebase was written as `[a, b]`. All of it was flagged Safe Correctable, so this is `bin/rubocop -a` with nothing done by hand. Every offense is gone. Formatting only. Ignoring whitespace, the diff touches five files: three blank lines and one trailing comma removed, plus a RedundantReturn in CoPlan::WebPushSubscription#device_label, where the `return` was on the method's last expression (the early-return guard above it is untouched). None of it changes behavior. Suite is unchanged: 1658 examples, 0 failures before and after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing was stopping the debt from coming back, so add a lint job to the CI workflow. It needs no database, so it is a checkout, setup-ruby and `bin/rubocop` -- action SHAs pinned to match the existing jobs. It runs as its own job rather than a step inside `test` so a bracket-spacing nit cannot mask whether the suite passed. Also state the db/schema.rb exclude explicitly. This one is belt and braces: rubocop-rails already excludes it via `db/*schema.rb`, which is why those 168 bracket offenses never showed up in a bare `bin/rubocop` run. Worth naming anyway, since it is the file people reach for first when they wonder why generated code is not linted. The `inherit_mode: merge` above it is load-bearing, not decoration. A bare `AllCops: Exclude:` replaces the inherited list rather than adding to it, which would have started linting bin/*, log/**, public/** and app/assets/**. Verified the change is inert: 425 target files before and after, with no file entering or leaving the set. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
RuboCop was installed but never enforced:
.github/workflows/ci.ymlhad no lint step, sobin/rubocophad drifted to 895 offenses across 75 files. 882 wereLayout/SpaceInsideArrayLiteralBrackets-- the repo usesrubocop-rails-omakase, which wants[ a, b ], and much of the codebase was written[a, b].Two commits, deliberately split so the mechanical churn is skippable.
1. Autocorrect the debt
Every offense was flagged Safe Correctable, so this is
bin/rubocop -awith nothing done by hand.bin/rubocopnow reports 425 files inspected, no offenses detected.Formatting only. Ignoring whitespace (
git diff -w), the 75-file diff reduces to five files:Style/RedundantReturninCoPlan::WebPushSubscription#device_labelThe
returnremoval is the only one worth a look. It was on the method's last expression, directly in the method body rather than inside a block, so dropping the keyword is inert -- and the genuine early-return guard above it (return "Unknown browser" if ua.blank?) is untouched.Files under
db/migrate/got reformatted too. That is safe: Rails tracks migrations by version number inschema_migrations, not by file checksum, so restyling an already-applied migration has no effect.2. Enforce it
A
lintjob: checkout, setup-ruby,bin/rubocop. No database needed, and action SHAs are pinned to match the existing jobs. It is its own job rather than a step insidetest, so a bracket-spacing nit cannot mask whether the suite passed.One note on the
db/schema.rbexcludeWorth flagging, because it is not what it looks like.
db/schema.rbwas already excluded --rubocop-railsshipsdb/*schema.rbin its ownAllCops: Exclude. Those 168 bracket offenses inschema.rbonly ever appear if you pass the path explicitly; they were never part of the 895, and-anever touched the file. The entry I added is documentation for the next reader, not a fix.The
inherit_mode: mergeabove it, though, is load-bearing. A bareAllCops: Exclude:replaces the inherited list instead of appending to it, which would have quietly started lintingbin/*,log/**,public/**andapp/assets/**. Verified the whole config change is inert by diffing--list-target-files: 425 files before, 425 after, nothing entering or leaving the set.If you would rather not carry a redundant exclude, the clean alternative is to drop both stanzas --
schema.rbstays excluded either way.Verification
bin/rubocop-- 425 files, 0 offensesbundle exec rspec-- 1658 examples, 0 failures, identical to the pre-change baseline on the same commitjobsresolves to[lint, test, test-postgres]Ruby 3.4.7 per
.ruby-version, MySQL, against an isolatedcoplan_lintdatabase.🤖 Generated with Claude Code