Pull Requestがいつまで経ってもレビューされない！
Author視点で、そのように思うことは多々あると思います。
そして、Reviewer視点では「レビューが大変そうだなぁと感じるPull Request」というものが、たしかに存在するとも思います。
そのようなときに「もっとこういった情報があれば、レビューが楽になるのに」と思うことも多々あります。
そこで、どのような情報がレビューコストを下げることができるか、過去の体験を元にリストアップしてみました。
Pull Requestが滞留することのデメリット
他にも沢山あると思いますが、少なくとも以下のデメリットがあります。
- 事業面:
- 施策・改善・バグフィックスなどのリリースが遅れる
- 開発者面:
- Pull Request作成後に時間が経過することで記憶が薄れ、masterとの乖離も大きくなっていくことで、追加の修正コストが増える
- コンフリクトする可能性が増える
- Author/Reviewerの精神衛生上よくない
逆にいうと、Reviewerのレビューコストを下げること、Reviewerに優しいPull Requestを作ることは、大きなメリットを持つのでは？と思います。
Reviewerに優しいPull Requestの作り方
基本的には、『Authorが実施したことをReviewerに実施させない』という考えに則っています。
『何度もやらせるって事は無駄なんだ・・・・無駄だから嫌いなんだ。無駄無駄・・・・』
※レビューコストを下げるためには「Pull Request分割して小さくする」という手段が最良の対応だと思うのですが、それはよく言われることですし、優れたエンジニアの記事が沢山あるので割愛しました。
Descriptionに情報を集約する
Descriptionだけ読めば意図と仕様が理解できるように、情報を整理・集約しましょう。
情報が複数のIssueやSlack上に散らばっており、「最終的な仕様が何かよくわならない！」というケースがあると思います。
また、「本当にこれでいいんだっけ？」と思うケースも多々あると思います。
特に経緯が複雑なものほど、そうなりがちです。
複数のドキュメントを行ったり来たりしながら最終的な仕様を想像していくのは骨が折れる作業です。
経緯を知っているAuthorが、経緯を把握していないReviewerに同じ作業をさせない方が良いです。
思考の流れを書く
「こう考えたので、この手段を取りました」ということを書きましょう。
コードの書き方ひとつとっても、そこには意図があるはずです。
Authorの考えをReviewerが想像するのは、非常に難しいことです。
自分がReviewerになったときに「なんでこう実装したんだろう」と思うことは誰もが経験したことでしょう。
その思考コストは本来不要なもので、Authorが伝えてあげるだけで解消できるものです。
Pull Requestへの行コメントはAuthorでも書くことができます。
ReviewerにAuthorの思考を伝えたほうが良いです。
「こう悩んだが、こうしました」ということも書きましょう。
何を優先して、今の書き方をしているのかを伝えましょう。
もし複数案があるのであれば、その案も書きましょう。
設計を図示する
複雑な構造になっている場合は、その構造を図示しましょう。
特に新機能を作る場合は、システムは複数のモジュールに分割されます。
そのようなときに、クラス図・シーケンス図・ER図などがほしいと思います。
全体構造を把握できていると、詳細な部分のレビューが楽になります。
Pull Request上にあらわれてくるコードはファイル名順であるため、上から順に見ていくことが「構造を理解する」ことに向いているわけではありません。
「どこから読んだらよいか」ということをReviewerが判断できる情報を追加しましょう。
ツールを使ってきれいに書く必要は無いと思います。
ノートやホワイトボードへの手書きを写真にとり、それを貼るというレベルで十分です。
スクリーンショットを貼る
見た目に変更がある場合、before/afterのスクリーンショットを貼りましょう。
css/jsやviewの変更は「実際に画面を見て判断したい」というものがほとんどだと思います。
そのような時にスクリーンショットがPull Requestに貼っていないと、Reviewerが自ら画面を開くことになります。
さくっと開ける画面であればまだしも、その画面を開くまでの手順が複雑なものであるほど、その画面を確認するのは大変です。
スクリーンショットによるレビューコストの削減が実現できます。
特に、以下のようにテーブル形式で比較可能なように整理すると良いです。
| \ | before | after |
|---|---|---|
| oo画面 | 画像 | 画像 |
| xx画面 | 画像 | 画像 |
類似のPull Request・ロジックのリンクを貼る
過去に実施した対応を横展開する場合など、その過去のPull Requestや現行のロジックのリンクを貼りましょう。
「正常に動作している実績があるものを踏襲する」というものは安心できます。
そういったものであれば、過去の対応内容と比較だけしてApproveすることができます。
横展開なのにその情報がPull Requestにないと、本来は不要なレビューコストを支払うことになります。
Reviewerが横展開だと知っている場合でも、リンクがなければ、元ロジックを探さないといけなくなります。
参照先のリンクを貼る
既存のロジックを利用する場合は、そのロジックの定義箇所のリンクを貼りましょう。
既存のロジックを呼び出している場合、その実装を確認したくなります。
エディタ・IDE上でのレビューであれば定義箇所に飛ぶのは簡単でしょうが、Pull Request上ではそうはいきません。
特に参照箇所が多いロジックの場合は、GitHubの検索でも複数候補が出てきます。
その定義箇所はAuthorが知っているはずなので、その定義箇所のリンクを貼りましょう。
同一リポジトリ内のソースであれば（もしくはPublicなリポジトリであれば）、「copy permalink」をすることで対象コードが展開されます。
影響範囲を示す
既存のロジックを変更する場合は、その影響範囲を書きましょう。
参照箇所が少ない場合は、そのロジックを参照している箇所のリンクを貼るのも良いと思います。
Authorは開発時にこれらをすでに調査ずみのはずですので、その情報をReviewerにも伝えてあげましょう。
テストケースを説明する
テストケースの全体像と、各ケースの意図をコメントに記載しましょう。
テストケースはロジックよりもコード量が多くなることもあり、レビューも結構大変です。
ロジックの確認が済んだあと『よし、あとはテストケースのレビューだけだ！』と思ったら、テストケースの方が巨大で滅入ってしまったということもあります。
Reviewerはテストのレビュー時に以下のようなことを考えます。
- テストのカバレッジは十分か
- このテストケースの意図は何か
これらのようなAuthorの頭の中にある情報を、Reviewerにも伝えてあげましょう。
最後に
理想的な状態は、Authorがコメントを書く必要がないレベルでシンプルなPull Requestになっていることだと思います。
すべてのPull Requestに対してそれを求めるのは酷なことだと思います。
様々な制約条件の上で、みんながPull Requestを作っています。
その制約を乗り越えるには、情報の透明性が一役買ってくれます。
同じコードでも、情報を添えるだけでReviewerのコストを減らすことができるのであれば、それに取り組む価値はあるかなと思います。