僕がrubocopに送ったPRが v0.47.0 に取り込まれました。

f:id:sue445:20170116111818p:plain

個人的に便利機能だと思うのでこの場を借りて軽く紹介したいと思います。

Rails/ReversibleMigrationについて
Cop開発Tips

Rails/ReversibleMigrationについて

Railsのmigrationファイルで change メソッドの中に書いたmigrationコマンドがreversible *1かどうかをチェックするためのcopです

具体例

https://github.com/bbatsov/rails-style-guide#reversible-migration より抜粋

これだと drop_table :users だけだと逆方向のmigration（create_table）でどのようなカラムでテーブルを作ればいいか分からないのでreversibleではありません。rake db:rollback や rake db:migrate:down した時にエラーになります。

# bad
class DropUsers < ActiveRecord::Migration
  def change
    drop_table :users
  end
end

こういう場合、upメソッドとdownメソッドでそれぞれmigrationを書いてあげないと rake db:rollback や rake db:migrate:down が正しく実行されません

# good
class DropUsers < ActiveRecord::Migration
  def up
    drop_table :users
  end

  def down
    create_table :users do |t|
      t.string :name
    end
  end
end

http://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters.html#method-i-drop_table にも書いていますが、 drop_table にブロックを渡すとchangeで逆方向のmigrationを補完する時に create_table の引数扱いになりカラムが作られます。

# good
# In this case, block will be used by create_table in rollback
# http://api.rubyonrails.org/classes/ActiveRecord/ConnectionAdapters.html#method-i-drop_table
class DropUsers < ActiveRecord::Migration
  def change
    drop_table :users do |t|
      t.string :name
    end
  end
end

どうして作ったか？

慣れてくればreversibleなコマンドかどうかは Railsの気持ちになって考えれば だいたい推測はつきます。

例えば

add_column :users, :name, :string

の逆は

remove_column :users, :name

ですが、

remove_column :users, :name

の逆方向を考えた時にどんな型でカラムを作ればいいかの情報がこれだけでは推測できないのでreversibleじゃないといった感じです。

微妙に迷った時も全部 http://api.rubyonrails.org/classes/ActiveRecord/Migration/CommandRecorder.html に書いてあるのでこれを読めば分かります。

しかしコードレビューの度にそれをいちいち指摘するのは面倒なので自動化するために Rails/ReversibleMigration を作成しました。

検出精度について

さっきのリファレンスを全部読んだ上でchangeメソッド内の下記を検出するようにしています

絶対にreversibleじゃないmigrationコマンド
- change_table
- change_table_comment
- execute
- remove_belongs_to
特定の条件を満たさないとreversibleじゃないmigrationコマンド
- drop_table
- change_column_default
- remove_column
- remove_foreign_key

ただし特例として reversibleブロックがある場合は reversibleであるとみなして検出対象外にしています。

詳細は下記参照

railsguides.jp

auto correctについて

rubocopのcopでは autocorrectメソッドを定義すればauto correct時の実装を定義することができますが、down（逆方向）のmigrationは過去のmigrationファイルを探して人間が書く必要がありそうな気がしたのであえて実装していません。

Cop開発Tips

次回以降自分がrubocop触りやすくするために開発手順とか参照すべきソースをメモっときます。

前提知識

日本語だとここが参考になりました。

koic.hatenablog.com

tech.sideci.com

rubocop静的解析したいソースをruby-parseでS式を出す

ruby_parser というRubyのソースコードを解析するためのgemを使います。rubocopのソースをcloneしていればbundle installで入るので別途gem installする必要はありません。

例えばこんなファイルを作っといて

class ExampleMigration < ActiveRecord::Migration
  def change
    # ブロックがあるのでreversible
    drop_table :users do |t|
      t.string :name
    end

    # ブロックがないのでrubocopで警告を出したい
    drop_table :users
  end
end

ruby-parseに食わせてS式を出します。

bundle exec ruby-parse /tmp/migration.rb
(class
  (const nil :ExampleMigration)
  (const
    (const nil :ActiveRecord) :Migration)
  (def :change
    (args)
    (begin
      (block
        (send nil :drop_table
          (sym :users))
        (args
          (procarg0 :t))
        (send
          (lvar :t) :string
          (sym :name)))
      (send nil :drop_table
        (sym :users)))))

rubocopのcopに on_def や on_send などのメソッドを定義することによって、rubocopで解析中にS式のその要素と対応するメソッドが実行されます。

今回の場合はメソッド呼び出し drop_table や change_table などのメソッド呼び出しをフックしたかったので on_sendメソッドを作りました。

binding.pryなどでとめてnodeの中身を確認

on系のメソッドに binding.pry や binding.irb を書いて

def on_send(node)
  binding.pry

任意のspecを実行

$ bundle exec rspec -- spec/rubocop/cop/rails/reversible_migration_spec.rb:85
Run options:
  include {:focus=>true, :locations=>{"./spec/rubocop/cop/rails/reversible_migration_spec.rb"=>[85]}}
  exclude {:broken=>#<Proc:./spec/spec_helper.rb:32>}

Randomized with seed 63344

From: /Users/sue445/workspace/github.com/bbatsov/rubocop/lib/rubocop/cop/rails/reversible_migration.rb @ line 120 RuboCop::Cop::Rails::ReversibleMigration#on_send:

    119: def on_send(node)
 => 120:   binding.pry
    121:   return unless within_change_method?(node)
    122:   return if within_reversible_block?(node)
    123:
    124:   check_irreversible_schema_statement_node(node)
    125:   check_drop_table_node(node)
    126:   check_change_column_default_node(node)
    127:   check_remove_column_node(node)
    128:   check_remove_foreign_key_node(node)
    129: end

[1] pry(#<RuboCop::Cop::Rails::ReversibleMigration>)> node
=> s(:send, nil, :drop_table,
  s(:sym, :users))
[2] pry(#<RuboCop::Cop::Rails::ReversibleMigration>)>

慣れていないうちは1回1回 binding.pry で止めてnodeの中身を確認しつつデバッグしていくのがいいと思います。余談ですがデバッグに慣れていなさすぎて1日 1migrationコマンド対応が限界でした('A`)

S式にマッチさせるmatcherを書く

cop内に

def_node_matcher :drop_table_call, <<-END
  (send nil :drop_table ...)
END

のようなmatcherを書くことによって、

drop_table_call(node) do
  # drop_table メソッドの呼び出しがあればこの中が評価される
end

のように処理を挟むことができます。

matcherの書き方は https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/node_pattern.rb のコメントを要熟読

matcherに $_ や $... を書くことにより、マッチした文字列を変数として取得することもできます。

$_ は単一nodeを取得

# remove_foreign_keyに渡された引数の2つ目のみを取得するmatcher
def_node_matcher :remove_foreign_key_call, <<-END
  (send nil :remove_foreign_key _ $_)
END

remove_foreign_key_call(node) do |arg|
  # $_ の内容がブロック引数のargに入ってくる
  if arg.hash_type?
    # remove_foreign_keyの第2引数がHash（テーブル名以外）の時に警告を出す
    add_offense(
      node, :expression,
      format(MSG, 'remove_foreign_key(without table)')
    )
  end
end

$... は以降のnodeを全部取得

# remove_columnに渡された引数を全部取得するmatcher
def_node_matcher :remove_column_call, <<-END
  (send nil :remove_column $...)
END

remove_column_call(node) do |args|
  # argsに引数全部入ってくる
  if args.to_a.size < 3
    # 引数が3個未満の時に警告を出す
    add_offense(
      node, :expression,
      format(MSG, 'remove_column(without type)')
    )
  end
end