Add Migration Execute Rule

[a-lavis]

Adds a new rule which recommends using execute instead of connection.execute inside migrations.

I think this rule should exist because execute gives useful terminal output while the migration is running, whereas connection.execute does not.

Corresponding RuboCop-Rails PR: rubocop/rubocop-rails#1589

Let me know if I should create a corresponding issue on this repo!

[andyw8]

I wonder how common this is. Could we do some checks, e.g. on real-world-rails?

The Rails docs show only execute being used:

https://guides.rubyonrails.org/active_record_migrations.html

[a-lavis]

I wonder how common this is. Could we do some checks, e.g. on real-world-rails?

The GitHub search: path:"db/migrate" language:ruby " connection.execute" (https://github.com/search?q=path%3A%22db%2Fmigrate%22+language%3Aruby+%22+connection.execute%22&type=code) brings up 197 files.

In real-world-rails, when I run find . -path '*/db/migrate/*.rb' | xargs ag --ruby -l ' connection.execute' | wc -l I get 56.

I recently implemented this cop for my closed source Rails repo at work, and it is working great! So I decided to upstream it in case others were interested in having it too.

In my closed source Rails repo at work (before I implemented the cop), find . -path '*/db/migrate/*.rb' | xargs ag --ruby -l ' connection.execute' | wc -l gives me 225 (out of a total of 1574 migrations). So definitely a lot higher than the average Rails repo I guess 😅.

The Rails docs show only execute being used:

https://guides.rubyonrails.org/active_record_migrations.html

Yeah, execute is definitely the way to go! Would be surprised if the docs featured connection.execute at all, since it doesn't have the benefits of execute mentioned in the PR (more details here: rubocop/rubocop-rails#1589 (comment))

[a-lavis]

In real-world-rails, when I run find . -path '*/db/migrate/*.rb' | xargs ag --ruby -l ' connection.execute' | wc -l I get 56.

Interestingly, when I run find . -path '*/db/migrate/*.rb' | xargs ag --ruby -l 'connection.execute' | wc -l (note the subtle difference of ' connection.execute' vs 'connection.execute), I get 178.

This is because of calls like ActiveRecord::Base.connection.execute and, for example, PublicBody.connection.execute (models like PublicBody inherit from ApplicationRecord which inherits from ActiveRecord::Base, so this is really also just equivalent to ActiveRecord::Base.connection.execute).

We could possibly make this style guideline / cop more useful if it also caught cases like this.

[pirj]

Much more clear now, thank you.

Is it correct to say that execute should be used when such output is needed, eg for schema changes, but connection.execute is preferable for data migrations?

Regarding omitting the receiver. How does it work with multi-databases?