Remove the RuboCop todo file and prune lint disables#1533
Merged
Conversation
The complexity and length metrics tracked an auto-generated Max that drifted for years and was never driven down. Lint/RescueException only ever fired on our deliberate pattern of rescuing Exception to record the error and re-raise it. Both produced noise instead of signal, so disable them and drop the inline disables that silenced them throughout the code. [skip changeset]
Style/Alias was excluded for these two files. The offenses are all the `alias :x :y` colon form and two `alias_method` calls in class bodies; rewriting them to `alias x y` is a safe autocorrect with no behaviour change, so apply it and drop the exclude. [skip changeset]
Naming/AccessorMethodName flags our deliberate set_*/get_* API naming, and Naming/PredicateMethod wants command methods that return a boolean renamed as queries. Both were only ever excluded or suppressed, so disable them and remove the inline disables and excludes they needed. [skip changeset]
|
Hi @unflxw, We've found some issues with your Pull Request.
|
There was a problem hiding this comment.
Pull request overview
This PR removes the auto-generated RuboCop todo file and replaces it with intentional, targeted lint configuration: disabling several cops that were either never enforced or conflict with established project conventions, and pruning now-unnecessary inline suppressions across the codebase.
Changes:
- Remove
.rubocop_todo.ymland stop inheriting it from.rubocop.yml, replacing todo exclusions with explicit cop disables and targeted inline exceptions where still needed. - Disable select cops globally (e.g., complexity/length metrics,
Lint/RescueException, naming cops) and remove related inline disables/excludes. - Apply small behavior-preserving rewrites/autocorrections (alias syntax, guard clause, numeric literal formatting, string concatenation, assignment-in-condition parentheses).
Reviewed changes
Copilot reviewed 39 out of 39 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
spec/support/testing.rb |
Makes keep_transactions? a strict boolean predicate and removes a naming-cop inline disable. |
spec/support/helpers/wait_for_helper.rb |
Removes inline suppression on rescue Exception in wait helper. |
spec/support/helpers/transaction_helpers.rb |
Removes inline suppression on set_current_transaction naming. |
spec/lib/appsignal/utils/data_spec.rb |
Replaces string + concatenation with array joining for expected JSON output. |
spec/lib/appsignal/transaction_spec.rb |
Updates numeric literal formatting to satisfy style rules without disables. |
spec/lib/appsignal/probes/sidekiq_spec.rb |
Adds targeted inline suppression for Struct.new override. |
spec/lib/appsignal/integrations/sidekiq_spec.rb |
Removes inline Lint/RescueException suppressions in specs. |
spec/lib/appsignal/event_formatter_spec.rb |
Adds targeted inline suppression around class variable reset in spec. |
spec/integration/runner.rb |
Adds parentheses to assignment-in-condition to avoid a lint disable. |
lib/puma/plugin/appsignal.rb |
Removes block-length suppression (metrics cops now disabled). |
lib/appsignal/utils/query_params_sanitizer.rb |
Adds targeted inline suppression for optional boolean parameter cop. |
lib/appsignal/transaction.rb |
Rewrites aliases to the plain alias x y form (Style/Alias). |
lib/appsignal/rack/body_wrapper.rb |
Removes inline Lint/RescueException suppressions. |
lib/appsignal/rack/abstract_middleware.rb |
Removes inline Lint/RescueException suppression. |
lib/appsignal/logger.rb |
Replaces todo exclusion with targeted inline suppression for missing super, and removes metrics disable on add. |
lib/appsignal/integrations/webmachine.rb |
Removes inline Lint/RescueException suppression. |
lib/appsignal/integrations/sidekiq.rb |
Removes inline Lint/RescueException suppression; adds targeted suppression for YAML.load. |
lib/appsignal/integrations/shoryuken.rb |
Removes inline Lint/RescueException suppression. |
lib/appsignal/integrations/resque.rb |
Removes inline Lint/RescueException suppression. |
lib/appsignal/integrations/rake.rb |
Removes inline Lint/RescueException suppression. |
lib/appsignal/integrations/que.rb |
Removes inline Lint/RescueException suppression. |
lib/appsignal/integrations/object.rb |
Rewrites a non-guard if into a guard clause. |
lib/appsignal/integrations/delayed_job_plugin.rb |
Removes inline Lint/RescueException suppression; adds targeted suppression for optional boolean parameter cop. |
lib/appsignal/integrations/action_cable.rb |
Removes inline Lint/RescueException suppression. |
lib/appsignal/hooks/active_job.rb |
Removes metrics disable and inline Lint/RescueException suppression. |
lib/appsignal/hooks/action_cable.rb |
Removes inline Lint/RescueException suppressions. |
lib/appsignal/helpers/instrumentation.rb |
Removes inline Lint/RescueException suppressions and rewrites aliases to plain alias. |
lib/appsignal/extension/jruby.rb |
Removes inline naming-cop suppressions for set_* methods. |
lib/appsignal/extension.rb |
Adds targeted inline suppression for missing super in MockData#initialize. |
lib/appsignal/config.rb |
Removes metrics disable and adds targeted suppression for YAML.load config loading. |
lib/appsignal/cli/install.rb |
Removes metrics disables (metrics cops now disabled). |
lib/appsignal/cli/diagnose.rb |
Removes Lint/ShadowedException suppression by simplifying rescue list. |
lib/appsignal/check_in/event.rb |
Removes metrics disable (metrics cops now disabled). |
lib/appsignal.rb |
Removes metrics disables (metrics cops now disabled). |
ext/extconf.rb |
Adds targeted inline suppression for predicate prefix cop on helper method name. |
ext/base.rb |
Adds targeted inline suppressions for Security/Open around open/URI.open. |
appsignal.gemspec |
Removes block-length suppression (metrics cops now disabled). |
.rubocop.yml |
Stops inheriting from todo, disables several cops globally, and documents rationale. |
.rubocop_todo.yml |
Deleted (no longer generated/used). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The remaining todo entries were each a single offense, not a file that categorically should skip a cop. Disable each one inline at the exact point instead, so the cop keeps protecting the rest of the code. Make keep_transactions? return false rather than nil so it is a real predicate. With nothing left to track, delete the generated todo file and stop inheriting it. [skip changeset]
A handful of inline disables marked code that a trivial, behaviour-preserving rewrite satisfies instead: a guard clause, digit grouping on a numeric literal, parentheses around an assignment used as a condition, dropping an exception class already covered by StandardError, and joining an array of strings rather than chaining `+`. Rewrite them so the disables can go. [skip changeset]
wait_for must rescue Exception, not StandardError: callers pass blocks containing RSpec expectations, and ExpectationNotMetError subclasses Exception, so catching only StandardError stops the retry from working. Add a comment saying so, and reword the Lint/RescueException disable note to describe our deliberate uses accurately rather than claiming the exception is never swallowed. [skip changeset]
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tombruijn
approved these changes
Jul 2, 2026
|
This is a message from the daily scheduled checks. |
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.
This is a follow-up to #1532, which upgrades RuboCop to version 1.87 by fixing some issues and generating a
.rubocop_todo.ymlfor the others. This PR removes.rubocop_todo.ymland cleans up.rubocop.yml. The intent is:.rubocop.ymlblanket file disables.As usual, with matters of the heart and mind and coding guidelines, there is space for disagreement. I went for bold, for reducing complexity, preventing noise, and highlighting genuine exceptions inline.
Stop running lint cops we never enforced
The complexity and length metrics tracked an auto-generated Max
that drifted for years and was never driven down. Lint/RescueException
only ever fired on our deliberate pattern of rescuing Exception to
record the error and re-raise it. Both produced noise instead of
signal, so disable them and drop the inline disables that silenced
them throughout the code.
Use the plain alias form for API aliases
Style/Alias was excluded for these two files. The offenses are all
the
alias :x :ycolon form and twoalias_methodcalls in classbodies; rewriting them to
alias x yis a safe autocorrect with nobehaviour change, so apply it and drop the exclude.
Disable two naming cops that fight our conventions
Naming/AccessorMethodName flags our deliberate set_/get_ API naming,
and Naming/PredicateMethod wants command methods that return a boolean
renamed as queries. Both were only ever excluded or suppressed, so
disable them and remove the inline disables and excludes they needed.
Replace the lint todo with targeted exceptions
The remaining todo entries were each a single offense, not a file
that categorically should skip a cop. Disable each one inline at the
exact point instead, so the cop keeps protecting the rest of the code.
Make keep_transactions? return false rather than nil so it is a real
predicate. With nothing left to track, delete the generated todo file
and stop inheriting it.
Drop lint disables a small rewrite makes moot
A handful of inline disables marked code that a trivial,
behaviour-preserving rewrite satisfies instead: a guard clause, digit
grouping on a numeric literal, parentheses around an assignment used
as a condition, dropping an exception class already covered by
StandardError, and joining an array of strings rather than chaining
+. Rewrite them so the disables can go.Document why we deliberately rescue Exception
wait_for must rescue Exception, not StandardError: callers pass blocks
containing RSpec expectations, and ExpectationNotMetError subclasses
Exception, so catching only StandardError stops the retry from working.
Add a comment saying so, and reword the Lint/RescueException disable
note to describe our deliberate uses accurately rather than claiming the
exception is never swallowed.