Making friends with RuboCop

Prathamesh Sonpatki -

At Memory.ai, we started using RuboCop heavily. This is a story of how we integrated RuboCop into our existing app.

This is not an introductory post to RuboCop. Check out what RuboCop is before diving into our experience report.

We started with rubocop, rubocop-performance, rubocop-rails and rubocop-rspec gems. We enabled all cops by default, this was our initial configuration in rubocop.yml

require:
  - rubocop-rspec
  - rubocop-rails
  - rubocop-performance

AllCops:
  EnabledByDefault: true
  TargetRubyVersion: 2.6.3
  Exclude:
    - 'app/views/**/*'
    - 'db/**/*'
    - 'bin/**/*'
    - 'csv/**/*'
    - 'slate/**/*'
    - 'vendor/bundle/**/*'
    - 'node_modules/**/*'

If you enable all the cops that come with RuboCop in an existing project, you will get tons and tons of warnings and offenses. This was pretty evident when we ran rubocop after adding above configuration.

Work in progress

For existing projects, there is a better way to integrate RuboCop. RuboCop provides a file .rubocop_todo.yml which records all the offenses from our codebase. We can fix these offenses one by one as we touch that particular piece of code. So we don't have to fix everything to start with.

To start using the .rubocop_todo.yml, perform following steps.

bundle exec rubocop --auto-gen-config
Added inheritance from `.rubocop_todo.yml` in `.rubocop.yml`.
Phase 1 of 2: run Metrics/LineLength cop
Inspecting 38 files
CC....C..C..C...C..CCC..CC....CC..C..C

38 files inspected, 40 offenses detected
Created .rubocop_todo.yml.
Phase 2 of 2: run all cops
Inspecting 38 files
CCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCCC

38 files inspected, 104 offenses detected
Created .rubocop_todo.yml.

You will see similar output but number of offenses might be overwhelmingly large or extremely less depending on the state of your codebase :)

This command did three things.

  • Inherited .rubocop.yml from .rubocop_todo.yml(We will come back to it later)
  • Ran rubocop on all of our code
  • Generated a new file .rubocop_todo.yml

Let's see contents of the .rubocop_todo.yml. It has recorded all the offenses present in our code. Let's see example of one of the cop for better understanding.

# Offense count: 1
# Cop supports --auto-correct.
# Configuration parameters: TreatCommentsAsGroupSeparators, Include.
# Include: **/*.gemfile, **/Gemfile, **/gems.rb
Bundler/OrderedGems:
  Exclude:
    - 'Gemfile'

This little piece of code tells us that our codebase has one offense for Bundler/OrderedGems cop and how we can fix it. But more importantly, it marks the file where this offense is present as excluded from the list of files on which RuboCop will be run.

What does this mean? Well let's try to run rubocop on the entire project now.

▶ bundle exec rubocop
Inspecting 38 files
......................................

38 files inspected, no offenses detected

Woah! Our code is clean, it passes all the cops, time for party!

Well, not really. This is where the comment Inherited .rubocop.yml from .rubocop_todo.yml needs to be looked at.

▶ cat .rubocop.yml
inherit_from: .rubocop_todo.yml

.rubocop_todo.yml records all the offenses of our codebase and also excludes those files from the list on which rubocop will be run.

rubocop.yml inherits from .rubocop_todo.yml so it also excludes those files from the list on which rubocop will be run.

When we run bundle exec rubocop, it picks up configuration from .rubocop.yml which in turn picks up the configuration from .rubocop_todo.yml which has excluded all the files which have offenses. All of this results into a green output from bundle exec rubocop command.

So we still have offensive code that RuboCop does not like, but we have a strategy to fix it incrementally rather than fixing it all at once.

Pro Tip: Always generate the .rubocop_todo.yml when you are integrating Rubocop into an existing project.

Git workflow

Next natural step was to act on the .rubocop_todo.yml file and fix the offenses as we touch the existing offensive code and make sure that the new code that we write is following the cops.

We decided to add a git hook which will run RuboCop on staged changes. RuboCop provides an option --safe-autocorrect which automatically corrects certain pieces of code based on whether a particular cop is considered safe for autocorrect or not. We leveraged this in our git commit hook so that developers don't have to worry about manually fixing all the offenses. When the machine can do it, why not?

We used husky and lint-staged npm packages to achieve this.

npm install --save-dev lint-staged husky

Add following in package.json

{
  "scripts": {
    "precommit": "lint-staged"
  },
  "lint-staged": {
    {app,spec}/**/*.rb": [
      "bundle exec rubocop --safe-autocorrect",
      "git add"
    ]
  },
  "devDependencies": {
    "husky": "^0.13.4",
    "lint-staged": "^3.6.0"
  }
}

This configuration makes sure that whenever a developer tries to commit code the offenses in that code which are deemed safe by RuboCop and the cops which support Auto Correct mechanism, are already fixed. After this our developers did not have to do anything extra apart from fixing offenses which are not considered safe by RuboCop manually.

Work in progress continues

Once the autocorrect git hook was in action, our list of offenses started coming down. We had to regenerate the .rubocop_todo.yml in between to get the accurate list of offenses. We decided against adding regeneration of .rubocop_todo.yml into Git hook as it was slow in our case.

.rubocop_todo.yml can be regenerated from the same command that generated it.

Happiness or Pain?

After all of this, we were expecting the offensive code to go down and code quality to improve day by day without any bugs. But we faced few challenges.

The autocorrect hook did some unexpected changes to our code in the process.

We had a piece of code which has a method update_attributes. rubocop-rails gem has a cop ActiveRecord/Aliases which changes calls to update_attributes with update. Though this change should happen only on Active Record models, the cop does not check whether the method is called on Active Record model or not. So in our case, it changed the call to custom update_attributes to update but did not change the method definition. It remained update_attributes. This resulted into error. As we had enabled safe-autocorrect Git hook, the developers came to know of this only when the build failed in CI.

We also faced issues with few other cops such as Rails/SaveBang and Style/StringHashKeys where they changed the code which was supposed to be not changed.

In the end, we decided to remove the safe autocorrect hook and relied on manual fixing of the offensive code.

Contributing back

When we faced the issues related to some of the cops which were not really safe for the safe autocorrect option, we tried to fix them by submitting patches to RuboCop.

https://github.com/rubocop-hq/rubocop-rails/pull/101

https://github.com/rubocop-hq/rubocop-rails/pull/98

https://github.com/rubocop-hq/rubocop/pull/7312

I will encourage everyone to do the same as it makes RuboCop better for everyone.

In conclusion, I will list down what we learned from this adoption.

  • Incremental adoption is key.
  • Autocorrect can be tricky, depends on your test coverage and lot of other factors, so don't use it if you don't have to.
  • Disable the cops that your team does not agree upon. We disabled cops such as RSpec/AnyInstance, RSpec/ExpectInHook, RSpec/AlignRightLetBrace
  • Contribute back to the community with your findings, code fixes so that critical tools such as RuboCop become better for everyone.

Want to know how we do Ruby and Rails at Memory.ai, subscribe to my newsletter here.