Pull Request の関心事は一つにしよう

こんにちは、大坪です。

自分はすごく小さくコミットをして小さく Pull Request にしていく癖があります。これによってピーク時は Findy 調べの年間コミット数で4位になりました。もちろんコミット数それ自体は大事ではないですし、コミットや Pull Request が小さい方が良いのかには議論がありますが、自分が良いと考えている開発スタイルの一つを体現するとこうなる傾向にあります。「関心事」という軸で小さい Pull Request を重ねる開発にまとめます。

この記事では git を前提にしていて、コードそれ自体に注目しています。Pull Request にするまでの仕事の進め方やディスクリプションの書き方などは下の記事を参考にすると良いでしょう。

また、Wantedly 特有の条件においてのみうまく機能するという提案も含まれます。Wantedly が前提にしている git の慣習については下の記事を参照してください。

Pull Request が持っていると嬉しい性質

よい Pull Request の分け方を考えるために、まずは Pull Request が持っていると嬉しい要素を列挙します。どれもそれだけを重視すると他のことが犠牲になる事があるという点に注意が必要です。あくまで「他にデメリットがないのであれば」という前提だということです。

diff の行数は少ないほうがいい

これは当たり前に実感できると思います。ただし diff を小さくすることだけを意識したコード変更が重なると設計全体の設計が良い状態でなくなってしまうことに注意が必要です。diff の行数が大きくなってしまう場合には「test を変更しない安心できる refactoringにする」「再現可能性を上げる」などを満たすと良いでしょう。これについては下のツイートとその関連ツイートに良い議論がまとまっています。

Test と実装は片方だけ変えるほうが良い

新しい実装の場合はこの前提は自明に成り立たないので主にリファクタリングにおける話です。なにか大規模なリファクタリングをする必要に迫られる場合は、「テストを厚くする」「テストに適合するようにリファクタリングする」を分けて行うと安心して merge ができます。テストと実装の両方が変わっていると挙動に変更がないかどうかが不安になります。

再現が簡単な方がいい

「再現が簡単であればどんなに大きな diff も入れて良い」という意見を筆者は持っています。例えば極端な例としては、レポジトリの運用にもよりますが、何百ものファイルにフォーマッターを書けることで数万行の diff が生まれるということも許容して良いと思っています。他には変数名がイケてないので変更したい、というときに分量が多くなってしまう場合は IDE の機能に頼らずワンライナーをシュッと書いてそれのレビューとたしかにそれを実行しただけで生成される diff であることを確認してもらう、という方法が良いと思っています。

「関心事」というメトリクス

ざっくりまとめた良い Pull Request が持っている性質の全てを取るのは難しいですし、他にも大事なこともたくさんあります。そこでざっくり持っていてほしい性質をまとめる概念が「関心事」だと思っています。

関心事とは Reviewer が気にすること一覧

明確な定義をするのは難しいので今回は「diff の前後で変わること」くらいのざっくりとした言い方をしようと思います。これがいくつあるかを簡単に測る方法としては、 Reviewer が確認するべき項目がいくつあるか？という考えて見ると良いでしょう。関心事が多いと「リファクタリング部分にバグがないか」「この diff は純粋にコーディングスタイルの変更のみか？」のように確認が分散してしまいます。他の数える方法としては Pull Request に「xxx を yyy した」の形式で行ったことを箇条書きにしてそれがいくつあるかを数えても良いかもしれません。

関心事を少なくする

例えば上の「diff が少ない」という性質を満たそうとすると一つのクラスやメソッドの変更にとどまり、「foo メソッドを高速化した」くらいの言い方ができるようになるでしょう。Test と実装は片方だけ、という話もそれぞれで関心事が違うので混ぜないほうがいいでしょう。「再現が簡単な方がいい」という主張もコードの diff という関心事をワンライナーなど他の小さなものに変換できているという意味で関心事を小さくできています。

自分はある機能を実装したいときに下のようなステップが見つかった場合、それぞれで一つの Pull Request にして出すようにしています。サクサクマージできるので気持ちが良いです。

• コードスタイル修正
• テスト追加
• リファクタリング
• 本当に入れたかった機能の実装

関心事を一つにする

関心事という単語の明確な定義は避けてしまいましたが、大事なこととしてこれは1未満にはならないという性質があります。Pull Request は小さい方がいいとしてもどこまで小さくするか？という話が出てきます。

それに対する自分の答えは、「関心事が増えないならできる限り詰め込んでしまえ」です。「レビュワーの負荷にならない(デグレする可能性が低い)」という前提があればどれだけ大きくてもいいよね、という感覚です。変更する箇所が多くても「全て同じ変更をしている」ということがわかっていればレビューの負荷は下げられます。

関心事を減らせないか？ということを考えるのがまず第一ステップとして大事ですが、関心事が1つまで減らせたら「この関心事に含まれるべきものは他にないか？」と考えるべきだということです。

Tips

最後にこの考えを実践するのに良いと考えている開発 Tips を紹介します。

とりあえずでっかいのを作って切り出す

とはいえ大規模な実装をするときに、特にリファクタリングをするときは最初から一つの関心事に絞って Pull Request を作るのが大変なこともあります。そういう場合に自分は関心事ごった煮の大きい Pull Request を Draft で作っています。その後適切な関心事の単位を見つけて切り出していくといいでしょう。git checkout <branch> <file> などを使うと別 branch の状態の file を checkout (気持ちの上では copy) できて一部楽になります。

Pull Request を chain する

あるやりたいことのためにn個の直列な PR を作りたくなったら、k + 1番目の PR の base branch を k番目の branch にするようにしています。そうすると diff が小さく見えるのに加えて、k番目が merge されると自動的に base branch が default branch に切り替わるので何もしなくても勝手にいい感じになります。関心事を小さくすると Review 速度のほうが PR 作成速度より早くなるので、新しい PR を積むときに一番古いものの Draft を外していくだけで良いペースで消化されていきます。