Rails / OSS パッチ会 2018年12月 だった

Rails / OSS パッチ会 2018年12月 だった。

ちょうど RuboCop で来ていたイシューがあったのでそのパッチを書いていたりした。

github.com

まずはいつものように再現ケースを書いて、テストが失敗するところから確認した。

    it 'accepts method call without trailing comma ' \
       'when a line break before a method call' do
      expect_no_offenses(<<-RUBY.strip_indent)
        obj
          .do_something(:foo, :bar)
      RUBY
    end

テストをパスさせるためのプロダクションコードの最初の実装は正規表現を使ったものだった。

実装前の方針として、まず .do_something(:foo, :bar) とメソッド呼び出しとその引数がシングルラインである場合は offense を出さないようにする方向で考えていた。

しかしながら以下のパッチでも示されている既存の multiline? メソッドはメソッドの引数に対するマルチラインだけではなく、obj.do_something の途中で改行されている場合もマルチライン扱いになる (シングルライン扱いではない) ところがひとつの発見で、multiline? メソッドだけの評価で真と返すものではなく何らかの条件と組み合わせて偽と返すようにするなどしたかった。

当初の実装は以下で、レシーバーとメソッド呼び出しが同一行にあるかどうかを正規表現を使って判定するようにしていた。

diff --git a/lib/rubocop/cop/mixin/trailing_comma.rb b/lib/rubocop/cop/mixin/trailing_comma.rb
index a106f8e6e..5037a0c5a 100644
--- a/lib/rubocop/cop/mixin/trailing_comma.rb
+++ b/lib/rubocop/cop/mixin/trailing_comma.rb
@@ -63,12 +63,18 @@ module RuboCop
         when :comma
           multiline?(node) && no_elements_on_same_line?(node)
         when :consistent_comma
-          multiline?(node)
+          multiline?(node) && !method_call_to_receiver_format?(node)
         else
           false
         end
       end

+      def method_call_to_receiver_format?(node)
+        return false if node.receiver.nil?
+
+        /#{node.receiver.source}\s*\.\s*#{node.method_name}/ =~ node.source
+      end
+
       def inside_comment?(range, comma_offset)
         processed_source.comments.any? do |comment|
           comment_offset = comment.loc.expression.begin_pos - range.begin_pos

正規表現を使うとだいたいダメなことは分かっていたものの最初の案はこれ。不吉な匂いの感覚どおりあいにく既存のテストが落ちることで断念した。

既存のテストが落ちる問題をテストケースにしたのが以下。

      it 'accepts a trailing comma in a method call with ' \
         'a single hash parameter to a receiver object' do
        expect_no_offenses(<<-RUBY.strip_indent)
          obj.some_method(
                            a: 0,
                            b: 1,
                         )
        RUBY
      end

複数行の引数の最後の閉じ括弧の手前でも改行しているパターンだった。

ここでどうやって解決したものか分からなくて本編は時間切れ、senny を囲んだ懇親会に行ってハートビートで2次会 (rails/rails の CI で落ちている問題について話しなど聞いていたりした) 、その帰り道に頭の中であーでもない、こうでもないと考えながら歩いていたら正解ではという問題解決の改善案が降りて来た。どうコードを書けば解決というものではなく .do_something(:foo, :bar) とメソッド呼び出しとその引数の終端の ) がある場合、ない場合までも含めて同一行であることを確認する方法を見つけることが正解ではという方針までだが、そこまで気づけばあとは紆余曲折のプリントデバッグの出番である (byebug でも良いが) 。

帰宅後にプリントデバッグしつつ、早速以下のような実装に進んでいった。

まずは以下のようにメソッド名の行と引数の最後の行が同一であること。これによってメソッドの名前から引数の最後までは同一行であることが判定できる。

      def method_name_and_arguments_on_same_line?(node)
        node.loc.selector.line == node.arguments.last.last_line
      end

次にメソッドの最後の行である ) のみかもしれない行とメソッドの最後の引数の行が一致していること。これにより引数の最後とメソッドの最後が同一行であることが判定できる。

      def method_name_and_arguments_on_same_line?(node)
        node.loc.selector.line == node.arguments.last.last_line &&
          node.last_line == node.arguments.last.last_line
      end

さらにこのケースは SendNode のみで起こるケースなので、ノードの判定を行なっている。これをしないと SendNode 以外のノード操作をする際に NoMethodError になりうる。

      def method_name_and_arguments_on_same_line?(node)
        node.send_type? &&
          node.loc.selector.line == node.arguments.last.last_line &&
          node.last_line == node.arguments.last.last_line
      end

早朝5時前くらいにパッチができたので PR を送ってパッチ会の成果となった。

github.com