@═╦╣╚╗ A mazing engineer
18-Apr-2019 Fixing a thousand of RuboCop offences quickly
In my current project we extensively use custom RSpec matchers. Some of which have countable qualifiers. Unfortunately, this specific piece of code is not reused, and I started looking at RSpec's internals in hopes it's there, and the only thing we need to do is include an RSpec::Matcher::Countable module to make the following code possible:
expect { perform! }.to send_notification(SpaceshipArrived).exactly(:twice)
Unfortunately, this module doesn't exist. Countable is used in rspec-expectations, and rspec-mocks, but its definition is spread out and not reusable. Hold my beer, I decided, I'll extract it. But there was another thing that immediately caught my eye:
$ rubocop
Inspecting 144 files
CCWCCCCCCCCCCCCCCCCCCCWCWWCCCCWCWWCCCWCCCCCWWWWWCCWCCWCWCCWCWCWCCCCCCWWWCWCWCCCCCCCCWCWWCCCWCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC
144 files inspected, 1324 offenses detected
WHAT?! I started looking, there were 66 different cops, each detecting from singular to over a hundred offences. Also RuboCop version is out of date, 0.52.1 released back in December 2017 vs 0.67 being the current version. Keeping in mind the very active development pace of RuboCop, that's a lot.
Hold my other beer, I decided, I'll fix them all.
Luckily enough, RuboCop is not only capable of detecting offences, it also comes with an option to auto-correct them. I could auto-correct everything, and if I would:
$ rubocop --auto-correct
144 files inspected, 1670 offenses detected, 1449 offenses corrected
2 errors occurred:
An error occurred while Layout/ElseAlignment cop was inspecting /Users/pirj/source/rspec-dev/repos/rspec-expectations/benchmarks/match_array/failing_with_distinct_items.rb:31:8.
An error occurred while Layout/ElseAlignment cop was inspecting /Users/pirj/source/rspec-dev/repos/rspec-expectations/benchmarks/match_array/failing_with_duplicate_items.rb:28:8.
$ git diff --stat
126 files changed, 1098 insertions(+), 887 deletions(-)
$ rspec
SyntaxError:
/Users/pirj/source/rspec-dev/repos/rspec-expectations/spec/rspec/matchers/built_in/eq_spec.rb:140: syntax error, unexpected end-of-input, expecting keyword_end
SyntaxError:
/Users/pirj/source/rspec-dev/repos/rspec-expectations/spec/rspec/matchers/built_in/exist_spec.rb:6: syntax error, unexpected '}'
$ git checkout spec/rspec/matchers/built_in/eq_spec.rb spec/rspec/matchers/built_in/exist_spec.rb
$ rspec
Finished in 5.22 seconds (files took 0.91972 seconds to load)
2247 examples, 2 failures, 5 pending
A couple things that I had to consider:
- it wouldn't be possible to push the +1090 -887 change through
- there were some failures
- some cops error'd out
- there were spec failures after the change, two syntax errors, and two failed examples
- the numbers of detected offences were off between normal and auto-correcting runs
My goal was to align the style used across the code base. However, to reduce the code churn, I would have to adjust the rules to the existing code.
It was tempting to update RuboCop to the latest version, in hopes the issues were fixed, but that would bring even more cops, and would require a lot of changes to .rubocop.yml, since cops keep chaning their names, and some of them retire. But most importantly, that would make rspec-expectations's RuboCop configuration misaligned with .rubocop_rspec_base.yml that is auto-generated and comes from an umbrella repository, rspec-dev, that has a base config for all RSpec related repositories, specifically mocks, expectations, core, rails, and support.
Yet another helpful RuboCop option to the help: rubocop --auto-gen-config. It creates a .rubocop_todo.yml file, disabling all the cops that have offences, including counts and configuration options:
# Offense count: 127
# Cop supports --auto-correct.
# Configuration parameters: EnforcedStyle.
# SupportedStyles: when_needed, always, never
Style/FrozenStringLiteralComment:
Enabled: false
Not bad. Mostly manual work from that moment. Run rubocop --only Style/FrozenStringLiteralComment, visually check offences, (ok, mostly auto-) fix offences, remove the cop from .rubocop_todo.yml, commit.
For some of the cops the change is just not worth it, like for the aftermentioned Style/FrozenStringLiteralComment. Only one file had frozen_string_literal: true directive set, so it was easier to just disable it.
There were cops, e.g. Layout/SpaceInsideHashLiteralBraces, with several different styling options, e.g. EnforcedStyleForEmptyBraces and EnforcedStyle, each with several options.
EnforcedStyle > | space (default) | no_space | compact
----------------------------- | - - - - - - - - | - - - - | - - - -
v EnforcedStyleForEmptyBraces | - - - - - - - - | - - - - | - - - -
----------------------------- | --------------- | -------- | -------
no_space (default) | 92 | 126 | 92
space | 105 | 139 | 105
In this case I went with the defaults, and 92 changes across the code.
One last option worth mentioning is rubocop --parallel, that, as its name suggests, processes in parallel. Even with relatively small code base the difference in execution time is noticable.
Of course, this journey wouldn't be interesting without some discoveries.
Back in the day when Ruby 2.2 was introduced, it started flooding the logs with "possible reference to past scope" warnings, and Myron Marston had to address them, as he later mentions "changed many variable and method names to names I do not like as much". The warning has later been removed, but the code in rspec-expectations remained. I had an opportunity to revert part of it, since RuboCop was pointing at it.
However, not all of them for some reason. I visually found a couple more that RuboCop missed. Temporary update to latest RuboCop indicated that this detection is not yet fixed in latest RuboCop version.
The impact on rspec-mocks was even more noticeable, as it caused 280 warnings across the test suite and urged the author to come up with undesirable changes:
- let(:dbl) { double }
+ let(:the_dbl) { double }
that left their traces until modern days.
64 commits later, a +576 −484 pull request including doc fixes, added TravisCI support et al came to life.