本記事は
【プルリクウィーク】
4日目の記事です。
💻
3日目
▶▶ 本記事 ▶▶
5日目
📚
こんにちは越川です。
GitHubはアプリケーションの開発に携わる人がメインで使う、という印象が強かったのですが現在、クラウドエンジニアの私もほぼ毎日GitHubを触っています。
私の場合、業務上、IaCを使うのでプログラミングをする機会が多く、その分プルリクエスト（以降PR）を見ることも出すことも多くあります。今回は自分自身がPRを見る時、または出す時にどんなことを意識しているのかを書いてみようと思います。
※PRとは新規開発や改修などの内容を関係者に通知する仕組みです。このPRをトリガーに関係者はコードのレビューを実施し、問題なければマージを行います。
※IaCとはInfrastructure as Codeの略称で、サーバーやネットワークなどあらゆるインフラリソースをコード化し、構築を自動化する手法です。
共通して重要なマインドセット
まず、見る側、出す側に共通して重要だと思うマインドセットを３つ挙げます。
作成者を詰問する場ではない
レビューを受けるとなると、矢面に立たされるイメージが強いですが、本来目指すべき姿としてはより良い物をチーム一丸となって作るという考えが重要です。 作成者一人だけであらゆる考慮点を洗い出すには限界があります。したがって、様々な意見や考えを取り込んで、成果物をブラッシュアップしていく作業が必要です。
また、作成者は自身の成果物に対して、愛着を持ちやすくなる傾向があります。したがって、客観的かつ多角的な視点を取り入れるためにも、レビューが不可欠です。
相手に確実にボールを渡す
PRを見る側（レビュワー）と、PRを出す側（レビュイー）がそれぞれ正確にボールの受け渡しをすることが重要です。 レビューの内容によっては複数のレビュワーを経由する必要があります。こういったケースではレビューのフローが途中で滞るリスクがあります。
具体的には、自分の番に回ってることに気が付いてない、またはレビューが終わっているのに関係者が認識していないなど様々です。Slack等に通知を自動化する方法もありますが、通知を受けた側が忘れてしまうケースも往々にしてあります。ですから、お互いが常に状況を確認し、必要に応じてリマインドを実施することが重要です。
起こり得る様々なコミュニケーションのパターン
トレーサビリティを意識する
チャットでやり取りをすることも多いですが、作成者の意図や編集の経緯をログとして残すためにも、やり取りはPRのConversation内で統一することが重要です。
※Conversationでは、ソースコードの指摘に対してスレッドを立て、関係者同士で議論できます。
チャットツールを挟んだ方が早く意思決定できる場合もありますが、最終的な内容をConversation内に明記しておくと、後でトレースしやすくなります。
ツールを跨ぐとトレーサビリティが失われるリスクがある
PRを出す時（レビュイー）
レビュワーの負荷を減らす
まず、レビュワーも複数のPRや別案件を抱えていたり、忙しいという前提に立って依頼をすることが重要です。したがって修正概要を記載し、見て欲しいポイントを簡潔にまとめることはレビュワーに対する配慮になります。これは一見、出す側からすると面倒ですが、見る側からすると非常に助かります。
GitHubにはissueのテンプレートや、PRのテンプレート機能が存在します。これらを使うことでフォーマットを指定することができるため、作成者に依存しないissueやPRを作成することができます。
レビュワーは他のタスクも抱えてるという前提に立つ
マージするまでが自分の仕事
共通のマインドセットで双方がボールの受け渡しを意識することが大事と記載しましたが、基本的にはレビュイーがレビュワーを巻き込んでマージまで持っていくべきだと考えています。規模によってはレビュワーの数も多くなる場合があります。
したがって、基本的にはレビュイーが全体的なレビューの進捗状況を確認し、どこかで滞っているようであれば関係者にリマインドします。 マージ担当が別にいるケースもありますが、いずれにしてもレビュイーが関係者を巻き込んでマージまで持って行ける状態にすることが重要です。
マージまで各関係者を巻き込むことが重要
マージ後の対応も忘れず実施
ブランチを削除したりissueをクローズするといった対応も重要です。削除を実施しないとゴミが溜まってしまいオペレーションミスの元になります。
Gitにはマージしたら自動的にブランチを削除する機能やissueをPRに紐づけて自動的にクローズする機能もあります。こういった機能を利用してゴミを作らない仕組み作りをするのも有効な策です。
また、マージしたら関係者に通知をすることも重要な作業です。とりわけ、依存関係のあるPRですと、他の開発に入れないケースもあります。そういったケースにおいては、作業を滞らせないためにも連絡することが重要です。
削除や通知といった事後作業も大事
PRを見る時（レビュワー）
まずPRを見る際は、基本的に何かしらの改善点がある、という前提で確認します。先述のとおり複数人でレビューを実施することで、考慮されていなかったポイント等の抜け漏れを炙り出します。軽微な修正であれば見る箇所も少ないですが、新規の機能実装やエンハンスであればその分、考慮点も多くなるはずです。
新規メンバーの気持ちで見る
レビューをする際は、新規で配属されたメンバーの気持ちになって見ることが重要です。プロジェクトのアサイン期間が長いと、良くも悪くも前提知識をベースにレビューしてしまいます。
とは言え、メンバーが入れ替わる可能性は多分にあるため、特定の人しか理解できないような記述は極力避けるべきです。したがって、できるだけ前提知識を捨てた上で、自分の中に疑問が出てこないかを注視してレビューします。
新規メンバーの気持ちで不明点が無いかを確認する
シンプルであるか
コードに限らず複雑な物はブラックボックス化を招き事故の元になります。私自身、一技術者として好奇心をそそられるような構成やテクノロジーを入れたくなることもありますが、まず何よりシンプルであるかを見るようにしています。
具体的には、コードの記載量がボリューミーでないか、冗長性が排除されているか、処理が複雑になっていないか。または、ファイル間に複雑な依存関係が無いか、などがチェックポイントです。常に人が入れ替わるという前提でシンプルさを追求することが重要ですね。
複雑な物は事故の元になる
保守性・拡張性が考慮されているか
リリース後も新機能が追加されたり、既存機能の改修など様々な使い方が想定されます。そういった場合に耐えられるように実装されているか？という観点も非常に重要です。長期的な目線であらゆる利用パターンを洗い出します。
例えば、クラスファイルを複数初期化した際にリソース名で重複エラーが起きないか、削除しようとした際に依存関係などは問題ないか、など沢山あるはずです。こういったパターンの洗い出しには一人では限界があるため、特にこの保守性・拡張性はいろいろな人の意見を取り込むことが重要です。
あらゆるユースケースを想定する
さいごに
今後は生成系AIなどの活用で、もっとコードレビューも効率化されていくと思います。とは言え、それでも抜け漏れは起きてしまうものなので、各人が今回記載したようなマインドセットを持ち続けることが大切かと思います。
こういったレビューのやり方は、プロジェクトによって結構ルールが異なるため、あくまで一事例として見ていただければと思います。一人でも多くの方に参考になると嬉しいです。
