はじめに
初めまして、バックエンドエンジニアの吉田です。
去年の2月からアンドパッドに参加することになり、約1年経ちました。
もともとPHPをメインとした開発をしていたのですが、Rubyに興味が湧いて2年ほど前からRailsのお仕事をしています。
アンドパッドに参画してからは、フロントエンド開発は未経験でしたが、幸運にもVue.jsのプロジェクトにも参加させてもらってRails+Nuxt.js環境での開発をしています。
この記事について
タイトルの通り、今回はANDPADのプルリクエスト（以下PR）について紹介していこうと思います。
ANDPADではソースコードの管理ツールとしてGitHubを利用しており、PRを作成して所属チーム内のエンジニア（時にはチーム外も）にコードをレビューしてもらっています。
PRについては各社様々なスタイルがあると思います。
ですが、意外と他の会社どのように進めているのかを知らない、または気になることがあるのではないでしょうか？
今回は僕がこれまでANDPADで見てきたPRの中から特に学びや気づきがあったものから一部を抜粋して皆さんにも共有したいと思います。
また、ANDPADのコードレビューについては下記記事でも紹介されていますので合わせてご覧ください。
tech.andpad.co.jp
ANDPADのPRから学んだこと
ANDPADのシステムは複数のリポジトリから構成されており、使っている言語も様々なものがあるのですが（Ruby、Vue.js、React、Goなど）、今回はRailsのリポジトリに絞って紹介していこうと思います。
以下はその中から4つを抜粋し、それぞれから得られたものをテーマをつけてまとめました。
1. より良い実装方法を教えてもらう
まず初めは、レビューをしてもらった際に、自分の実装方法よりも良い方法を教えてもらえた時のものです。さらにそれが自分の知らない、または自分では思いつけない実装方法だと僕はとても得をした気持ちになります。
このパターンは普段レビューをしてもらう時に度々経験するのではないでしょうか？
紹介するPRは以前僕がRubyのmoduleを作成した時のものです。
そのmoduleは、ある設定値を定数で持っており、それをincludeした先で上書きすることでmoduleで実装された機能の挙動が変わるような実装でした。
その実装に対してtricknotesさんから下記のようなレビューが。
『「定数」は「定数」なので』こういった単純なことが自分で実装している時には中々気付けないことがあるので、これは言われてみて確かにという感じでした。。
（Rubyではサブクラスで同じ定数を定義した場合は上書きされる）
また、レビューではこの実装の代替案も記述されていました。
cattr_accessorについては知らなかったので、調べてみるとクラス変数の値を親クラスから子クラスに継承する時に使えるようで、確かにこれを使ってやりたいことは実現できそうです。
（ちなみに実際の実装ではインスタンス変数から値を変更できないように、instance_writer: falseオプションを追記しています）
このように、自分一人ではなかなかたどり着けない情報を、言語やフレームワークの熟練者から教えてもらえるのはとてもありがたいです。
2. 気付いていなかった観点を得る
次はActiveDecoratorを使用していた時に、思わぬ挙動をしたため、調査&コード修正対応をした時のものです。
具体的には、Deviseで設定されるcurrent_userに対してActiveDecoratorがdecorateできていないという問題がありました。
この時、僕はActiveDecoratorのコードを読んでみて原因や回避方法を探るというアプローチをして、問題は解決することができたのですが、レビュー内で予想外の指摘をもらいました。
レビュワーのkinkinkon1009さんからのコメントには、僕が調査していた問題に関連するIssueのURLが貼られていました。
リンクを開いてみると、僕が遭遇したのと同じ問題について議論がされていて、さらに解決方法の提案も書いてありました。
ライブラリの挙動に疑問を持ったらコードを読むだけではなく、Issueも調べてみることで解決策が得られることもある、ということを教えてもらいました。
3. レビュワーが興味を持てるよう工夫する
次は、松田さん作成のPRで、Capybaraのバージョンを2系から3系に上げた時のものです。
このPRが作成された当時、ANDPADではRails 5.1系を使用しており、Rails内で下記の様にCapybara 2系をロードしているので、3系にバージョンアップするためにその制約を回避するパッチを追加した、というのがこのPRの主な内容です。（Rails5.1の下記コード部分のURLはこちら）
gem "capybara", "~> 2.13"
このPRの見所はこの対応内容とその説明部分も面白いということですね。その中から一部を抜粋します。
Rails 5.1上でもCapybara 3以上を使いたければ、ちょっと無理やりなハックが必要になります。…というのが、このぷるりのちょっとおもしろいところです。
gem "capybara", "~> 2.13" っていう設定のような宣言のようなDSLは、もちろんRubyなのでふつうのメソッド呼び出しなわけで、その実体は Kernel#gem になります。
ここでは、capybaraの不要なバージョン制約を取り払って、素直にbundlerでbundleされたものをロードするようにしたい、という要件なので、Kernel#gemに "capybara" と謎のバージョン制約が渡されたらバージョン制約のほうを単に無視する、というモンキーパッチで対処することができます。こんな感じで、正しく使えば世の中を良くすることができるフォースがユーザーに与えられているのがRubyの良いところですよね。
下記は本PRで対応されたコードのメイン部分です。config/initializersディレクトリ配下にパッチを作成して、下記メソッドを追加してまいす。
説明通り、capybaraに~> 2.13が指定してある場合はバージョン指定を無視してロードしているのがわかると思います。
def gem(gem_name, *requirements) if (gem_name == 'capybara') && (requirements.first == '~> 2.13') gem gem_name else super end end
このPRは対応内容だけでなく、読み物としても面白いものだったので最後まで引き込まれるように読んでしまいました。
PRの説明を記述する際はついつい事実の列挙になってしまいますが、そのコードを書くに至った背景や、時には書き手の気持ちを入れることで伝わり方が全然違うというのは新らしい発見でした。
4. 開発者フレンドリーな仕組みにしておく
引き続き先程のPR内からの抜粋です。上記パッチはRails5.1専用の対応のため、Rails5.2以上へのバージョンアップ以降は必要ないコードになります。
そのため、パッチの冒頭に下記の様に、Railsのバージョンアップ後に起動すると例外を投げる記述がされていました。
raise 'This monkey patch is for Rails 5.1. Please consider removing this file.' unless Rails::VERSION::STRING < '5.2'
この記述により、ファイル削除を忘れることがなくなるので、開発者はコードが不要になった時点で気づくことができ、無駄なコードを残さないで済みます。
これはとても参考になりました。
おわりに
今回はANDPADのPRについて紹介させてもらいました。
上記以外にもまだまだ紹介したい良PRはたくさんあります！！
そして日々増え続けてます。
これを書くにあたり、良いPRはブックマークしておけばよかったというのが反省点ですね。（後から思い出して探すのは割と大変。。）
今回はRailsのリポジトリに絞って紹介させてもらいましたが、Rails以外のリポジトリでも勉強になることが多々ありますので興味を持っていただけた方は一緒に研鑽していきましょう！
