Pull Request をレビューするときにやっていること

エンジニアの村松です。先日、自宅でバグを発見しました。リアルバグです。某 DB の名前にもなっているアレです。今もビクビクしながらこれを書いています。

はじめに

さて、アソビューでは GitHub の Pull Request (以降、PR と書きます) を活用して開発を進めています。PR のレビューは、コードの品質担保や、(レビュイーの属人化を防止する目的も含めて) 仕様や設計を担当者以外が理解するために重要なプロセスであると考えています。

私自身、前職を含めてプロジェクトのリーダーを担うことが多く、PR をレビューする機会が多くありました。(油断すると1日で10件以上溜まることも...)

これまでに多くの PR を見てきた中で、自分の中で自然と身に付いたルーティンのようなものがあるので紹介します。他の人が普段どうやって PR を見ているのか気になる方の参考になれば...。

いきなり diff (Files changed) は見ない

開発メンバーから PR のレビュー依頼が来て PR を開いてもいきなりコードの diff (Files changed) を見ることはしません。

いきなり diff を見ると変更した箇所だけに着目してしまい、一見するとその変更内容が正しいとさえ思えてきます。(短絡的にこれが正しいというバイアスが掛かるような気がします)

これでは全体を俯瞰した効果的なレビューができなくなってしまいます。では、どうするのがいいでしょう。私は次のようなステップを踏みます。

PR を開いて最初にやること

基本的なところですが、まずは PR の description を読みます。この PR の変更には、どういう背景があり、どういう目的で、どのような方針で変更したのかを把握するようにします。description に書いてある内容で事足りることもありますし、必要に応じて仕様書や設計書などのドキュメントを確認します。

そのため、関連する資料がある場合には PR にリンクが張ってあるとレビュワーとしてはとても助かります。(逆にあまりにも情報が少ない場合には description を書いてもらうようにレビュイーに依頼することもあります)

不具合修正に関する PR の場合は、不具合の内容や再現手順、原因、修正方針を確認します。

diff を想像する

この段階である程度 PR の概要は把握できますが、まだコードの diff (Files changed) は見ません。

私はここで diff を 想像 します。

自分だったらどのように変更するか、この手の変更だと◯◯のようなパターンの考慮が必要なのではないか、この機能に変更を加えるとなると〇〇の機能にも影響があるのではないか、など少し時間を取って考えられる範囲で考えてみます。

diff を読む前にこのあたりを考えることで、バイアスに依らずクリアな頭で影響範囲などを考えることができます。

diff を読む

ここまでの段階を経てようやく diff (Files changed) を開きます。

自分が想像したものと同じような diff になっている部分もありますし、異なる部分もあります。自分の想像と異なる部分は重点的に読みます。特に、読み解けなかった部分についてはレビュイーに確認します。

PR のレビューは誤りを指摘するだけではありません。レビューを通して仕様や設計を理解し、レビュイーの属人化を防止するためのコミュニケーションツールだと思っています。分からないところは遠慮せず質問するようにしています。もちろん、改善案や代案を提案することも重要です。

また、適切ではないと感じる命名や typo など、細かくても遠慮せず指摘します。(統一感のない命名や typo はあとで grep しにくくなって困るので...)

Pull Request をどこから読むか

PR の変更ファイル数がある程度のボリュームになると、どこから見ていくか迷うことはないでしょうか。私は次の3パターンで読むことが多いです。(3パターンを行ったり来たりすることもあります)

トップダウンで読む

例えば、API の新規追加などであれば、エンドポイントのようなエントリーポイントとなるところがあるはずです。そこから読み下していきます。トップダウンで読むと機能単位の構造が理解しやすくなります。

ボトムアップで読む

リファクタリングや性能改善のような既存機能の改修で、特に SQL の変更などが多い場合には、SQL や Repository レイヤから読み始めて、足回りの理解を固めるところから始めます。この場合、事前に変更の背景や方針を把握してレビュイーとの目線を合わせておくのが重要です。

コアから読む

機能の拡張などで新しい概念が追加されるような場合には、ドメインモデルから読み始めます。ドメインモデル (登場人物) を把握することで、その前後のロジックが想像しやすくなります。そのあとはトップダウン or ボトムアップで読み進めます。

PR レビューのポイント

冒頭でも書いたように、コードの diff だけ (変更した部分だけ) を見ても、その変更内容が正しいかどうかは判断しにくいことがあります。

Files changed の Expand Up/Down/All で変更箇所の前後のコードを見たり、手元でブランチを checkout して変更箇所の呼び出し元や呼び出し先を見ることも重要です。

また、ローカルに checkout しなくても、PR の画面で . キーを押して github.dev エディターで PR を開くことができます。PR が少し読みやすくなります。

docs.github.com

あと、みなさんも活用されてるかもしれませんが、diff の右上にある Viewed チェックボックスをよく使います。PR の Files changed を行ったり来たりしているとどこまで読んだか分からなくなることがあるので、この Viewed チェックボックスにはよく助けられています。

おわりに

今回は僭越ながら私が普段どのように PR をレビューしているのかを紹介させていただきました。自分はこういう風に見てるよ!というのがあればぜひ教えてください!!

プログラミングは書くことにフォーカスされがちなように思いますが、個人的には読む力も重要であると考えています。PR をブラウザで見る限りは結局はテキストで読むことになるため、やはり目コンパイルする力が大事です。脳内のコンパイラを鍛えましょう!

最後に

アソビューでは「生きるに、遊びを。」をミッションに、一緒に働くメンバーを募集しています!ご興味がありましたらお気軽にご応募いただければと思います!

www.asoview.com