プルリクエスト、ちょっとだけわかった

はじめに

これはプルリクエストをほぼ使ったことがなかった僕がプルリクエストをゴリゴリに使ってる現職で得た知見をまとめた記事です。
プルリクエスト使ったことないよっていう人はぜひ導入の足がかりにして欲しいし、既にプルリクエスト使ってるよっていう人も何か参考になるとこは拾ってもらえると嬉しいです。

プルリクエスト、たぶんこうするといいよ

心構え編

そもそもプルリクエストを出す側（レビュイー）とレビューする側（レビュアー）がどういうお気持ちでプルリクエストに向き合うべきなのかっていうところですね。ここを履き違えるとプルリクエストの度にお互い変な緊張感に包まれることになります。

「指摘をすること」を目的にするのはやめよう

大前提としてプルリクエストの目的は「指摘をすること」ではないことをレビュイー・レビュアー共に共通認識として持っておくべきだと思います。「指摘をすること」は過程であり、目的はその先にある「よりよいコードにすること」であるということを心に留めておくと、ともすると無機質に見えがちなレビューの先に「よりよいコードにする」という共通の目的を見出すことができるようになると思います。
あと、これは完全にお気持ちの問題ですが「指摘」はマイナスな感じですけど「よりよいコードにするための提案」はプラスな感じがしますよね。

心理的安全性を保とうと努力しよう

心理的安全性ってなんだ？って方は各々調べてもらうか、リクルートさんがいい感じまとめてくれているやつを読んでもらうと大体分かるかと思います。
簡潔にまとめたら「自分のアウトプット（これは意見だったり質問だったり、自分がやった作業内容、書いたコードやプルリクエストも含まれると僕は思ってます）を安心してチームに共有できる状態」が「心理的安全性が保てている状態」だと僕は解釈しています。
例えば、どう考えても効率的じゃない書き方してたり、そもそも文法間違ってたりみたいなコイツ本当に動かしてテストしたのか？みたいな時でも

ここパフォーマンス的に問題あるから書き直して。
あと、ここって文法ミスってるからそもそも動かなくない？

みたいな書き方じゃなくて

〇〇という書き方するとパフォーマンス面の問題が解決できるかもしれないので、そっちでも実装してみて処理速度の比較をお願いします。
あと、〇〇の箇所が文法的に誤っているので修正お願いします。

みたいな感じであくまで提案ベースで書くとあんま責められてないなというお気持ちになると思います。
また、レビュアーが必ずしも該当の作業内容やドメインに詳しいとは限らないので、レビュイーもレビュアーの指摘が全くの見当違いでも強い言葉で責めないようにしましょう。

プルリクエスト作成編

プルリクエストを作る際のこういうところ気をつけた方がいいと思うよっていうところをいろいろ書いていきます。
主観もいっぱいだから参考にできるところだけ参考にしてね。

プルリクエストは小さく分けた方がいいよ

これに尽きます。
僕が現職で初めて作ったプルリクエストは変更ファイル20個越え・変更行1000行超えみたいな超弩級プルリクエストでした。
結果、あらゆるメンバーからあらゆる箇所への指摘が1つのプルリクエストに書き込まれ、さらにそれに対する変更を同じブランチにコミットしていき、そのコミットに対するコメントが追加で書き込まれ...みたいな感じで、プルリクエストがウィンチェスターハウスみたいになってました。

そもそもデカいプルリクエストを作成することの何が悪いのよ

そもそもデカいプルリクエストの何が悪いのかっていうことが僕も理解できていませんでした（というか自分のプルリクエストがデカいという認識がまずなかったんだけどね）。
簡潔に言うと「正確なレビューができない」ことが問題になると思っています。
1つのプルリクエストにあまりにも多くの作業内容が含まれていると、レビュー箇所もレビュー内容も分散してレビューの精度が落ちることが懸念されます。
あと普通にレビュアーが疲れると思います。デカいとレビューが重いし時間も取られるので。
正直、レビューお願いしますって言って渡されたリンク踏んだら超弩級だったときのチームメンバーの気持ちを考えたら涙が出てきます。僕なら一旦忙しくて見れなかったことにします。
そして、場合によってはレビュイーも疲弊します。
せっかく自分ががんばって作ったかわいいロジックたちに対してどんどん指摘が飛んできたら、人によってはとても落ち込むと思うし、それで自己肯定感が下がっちゃうからプルリクエスト上のレビューコメントたちがだんだん自分の人格を攻撃しているように見えてくるっていう人もいるかもしれません（僕のことじゃないよ！）。
お互い傷つかないためにプルリクエストは小さく作るようにしましょう。

プルリクエストを小さく作るってなんだよ

そもそもプルリクエストを小さく作るってなんだよって人がいると思うんですけど、例えば1つのプルリクエストで何をしたのかと言うことが簡潔に説明できない場合は粒度が大きすぎると判断して良さそうというのが僕の体感です（Gitのコミットと一緒だね）。
例えば、1つのプルリクエストの作業内容が「TODOを登録するAPIを作ってそれを呼ぶフロントエンド処理も書いた」だと、「APIの実装」と「フロントエンドの実装」の二つをレビューしないといけなくて、これって実は二つのプルリクエストに分けれたんじゃない？ってことですね（場合によってはもっと細かく分けれるかもね）。
ちなみに、超弩級プルリクエストで鮮烈なプルリクエストライフデビューを果たした後の僕は「アーキテクチャに沿ったスケルトン作りました」とか「APIの正常系の実装しました」とかの粒度でプルリクエストを作るようにしました。
こうすることで「アーキテクチャの構造に沿ってないとかクラス名イケてないんじゃないについてのレビュー」と「正常系の処理の実装に対するレビュー」が別々のプルリクエストで行われるようになるのでレビューする側もレビューされる側も負担が減って辛くなくなります。
あと、一般的に（というか実際に言われたんだけど）いじってるファイル数が10ファイルとか超えてくるとデカいなぁという気持ちになると言われます。
この辺は例えばGitHubならDraftでプルリクエストとか作ると変更ファイル数とか行数とか客観的に見やすくていい感じなので、そういうのも駆使しながらなるべく小分けにしていくといいと思います。

実際の運用どうやってんのよ

実際に小さいプルリクエストでどうやって運用してんのよって話ですよね。
正直僕も現職に来るまでちゃんとプルリクエストでレビュー回してるところってどんな感じでやってるんだろって思ってました。
いまの僕のやり方は、例えば機能開発ならtask1_base_branchみたいなベースになるfeatureブランチを切って、そこからtask1_make_todo_apiみたいな細かい作業ブランチを切る感じでやってます。
で、task1_make_todo_apiでの作業が終わったらtask1_base_branch向けにプルリクエストを作成する感じです。
こうすると都度都度レビューされた小さなプルリクエストを統合したベースブランチ（task1_base_branch）が出来上がるっていう寸法です。
最後は出来上がったベースブランチをdevだかstagingだかmasterだかにマージしていく感じですね。

プルリクエストの説明文はちゃんと書いた方がいいよ

プルリクエストを出すときに説明文に何も書かずに出すと何をレビューしたらいいのかレビュアーも全然分からないから絶対に説明文は書いた方がいいです。
あとこれは観点として忘れがちなんですが（事実、僕も言われて気がついた）、後世の人たちが参照する可能性があるのでそのときのためになるべく情報を残しておこうという観点です。
将来的に、今回いじったあたりでバグが発生しましたとか、新機能追加することになりましたとか、なんでこれこういう実装になってるんだっけとか、そう言ったときになぜいまの実装はこうなっているのかという履歴がプルリクエストという形で残っていると、後世の人たちの調査の手助けになりますよね。
とは言ってもフリースタイルだと何を書けばいいんだっていう人も多いと思うので、例えばGitHubならこの辺を参考にするといい感じにチーム共通のテンプレートを作成できます。
そしてテンプレートに何を書けばいいのかという人のために、実体験を元に最低限これは書いた方がいいんじゃないかな項目を書いておきます。
実際に何を書くべきかはここに挙げた内容以外のことも含めて各々のチームで精査してください。

どのチケットと対応しているか

何かしら別のサービスでタスクを管理している場合はそのチケットのリンクを貼っておけば、最低限のこのプルリクエストが何の作業をしているかということは伝わるので貼り付けておくといいと思います。
あと細かい説明はチケットの方に逃して、プルリクエスト上では実装のことに焦点を絞って書くみたいな使い分けができるようになります。
ついでにチケットの方にプルリクエストのリンクを貼っておくと相互参照性が上がっていい感じになると思います。

やったこと・やらなかったこと

今回のレビューで「やったこと」と「やらなかったこと（あれば）」を記載しておくとレビューの助けになると思います。
例えば、ある機能の開発をしているときに既存のロジックのバグに気がついてしまったとします。
本当はこれを修正すべきなんだけど、その作業を今のタスクに含めると作業内容が大きくなりすぎてしまうなというお気持ちになると思います。
そういうときに、「やったこと」は「今回のタスクの内容」、「やらなかったこと」は「これこれこういうバグの修正」みたいな感じで書くと、どこまでがレビュー対象なのかがレビュアーに通じやすくなります。
ついでにもっと気が効く人は「やらなかったこと」を解決するために切ったのチケットのURLとか貼っとくといいかもしれません。

エビデンス

UIの変更とかだったら変更前と変更後でどう変わったのかがスクショとか映像とかGIFとかで貼り付けてあると大体どういう動きなのかが分かるし、UIの実装面での認識のズレとかも指摘できるからあると便利だと思います。
ちなみに僕はGIFでエビデンス上げる時はGIFHY CAPTURE使ってます。手軽に画面をキャプチャしてGIF化できて便利です。

再現手順

これ本当に大丈夫なんか？みたいなとこは手元で実際に確認してもらった方が間違いないので再現手順とかも書いとくといいと思います。
このアカウントで入ってこの操作をすると該当の現象を起こせるよみたいな感じですね。
手順は箇条書きでステップごとに書くと分かりやすくていいと思います。

レビューの優先度

いくつかレビューを投げてたらどれをどのくらいの優先度で見たらいいか分からないみたいな状態になってしまう可能性があるので、そのプルリクエストの優先度を記載するようにするといいと思います。
いちいちプルリクエスト開かないと優先度が分からない仕様だと面倒だと思うので、GitHubだとプルリクエストにラベルをつける機能があるのでそれを使うといい感じに開く前から一覧画面で優先度が視認できるようになると思います。

レビューコメント編

レビューするときのコメントでここを気をつけた方がいいと思うよっていうところをいろいろ書いていきます。
ここも主観がいっぱいだから参考にできるところだけ参考にしてね。

バッジをつけよう

例えば

ここは〇〇という書き方があるのでそっちの方がいいかもしれません。

みたいなコメントが書いてあったときに、絶対直してほしいのか、それともできれば直してくらいなのか、どれくらいの情熱をもって書かれたものかが分からないというときがままあると思います。
そういうときのために「このコメントはこういう意図で言ってるよ」というのを他のメンバーに伝えるためにバッジをつけるようにするとコミュニケーションが楽になると思われます。
ここのバッジの文字の意味は意外と誰もわざわざは教えてくれないスラングみたいな感じで、メンバーに「IMOって何？」とか聞くのが女子高生に「ぴえんって何？」って聞くみたいでちょっと恥ずかしかったりしたので、知らない人はこっそり習得できるようによく使われるやつを下に列挙しときます。

バッジの文字	意味
MUST	「絶対に直してね」の意味。要件が満たせていなかったり、確実にエラーが起きるときとかに使われる。直してくれという情熱が最もアツい。
FYI	「参考までに」の意味（For Your Informationの略らしい）。人によってはSUGGESTIONだったりする。言語とかフレームワークの仕様でこういう書き方ができるよみたいなときに使われる。
IMO	「私が思うには」の意味（In My Opinionの略らしい）。みんなイモって読んでる。僕が書くならこうするなくらいの情熱。
ASK	疑問や質問があるときにつける。人によってはQとかQUESTIONとかだったりする。
NIT	細かい指摘のときにつける（Nit Pickの略らしい）。人によってはNITSだったりする。ここフォーマットがズレてますねとかの指摘のときについたりする。
COMMENT	特に実装に直接関係はないコメントのときにつける。ここってそのうちどうにかしたいですよね〜みたいなときに使われたりする。
GOOD	いい実装があったときにつける。レビュイーとしてはなるべくこれだけ見ていたい。

これを例えば

[IMO]
ここは〇〇という書き方があるのでそっちの方がいいかもしれません。

みたいな感じで使うわけですね。

ちなみにこのバッジをかっこよくいい感じのバッジで表示するShield.ioっていうサービスがあるので気になる人は使ってみてください。
こんな感じ

GitHubならShield.ioで作ったイケてるバッジを返信テンプレートに登録しておくと使いやすいです。

いいところは褒めよう

レビューが嫌になるのは、する側は悪いところを探さなければならない、される側は悪いところを指摘される、という先入観のせいだと僕は思っています。
なので、可能ならいいところを見つけて褒めるように心がけると精神衛生上とてもいいです。
自分のアウトプットを褒められて嫌な人は恐らくいないので積極的に褒めていくといいと思います。
コードで褒めれるところは意外といっぱいあって、例えば前回指摘したところをちゃんと今回は拾えていたとか、すごく分かりやすいメソッド名・変数名を付けているとか、崩れていたフォーマットをいい感じに直してくれたとか、後続に役立つコメントを残してくれたとか、単純にめちゃめちゃ実装がイケてるとか...。
そういうときに、上に挙げたGOODバッジをつけて

[GOOD]
変数名分かりやすいですね👍

みたいなのを書いてもらうとレビュイーは嬉しいと思います（少なくとも僕は嬉しいね！）。

理由をちゃんと書こう

コメントを書く時には基本的に「なぜ」が伝わるように書くように心がけましょう。
指摘や別の方法を提案するときに「〇〇なのでこうした方がいいです」といった理由があると、レビュイー側も納得感を持って修正に取り掛かれると思います。
また、上でも書いたようにレビュイーもレビュアーも必ずしもそのドメインや実装について詳しいとは限りません。「先人がこう言ってるんだからあってるんだろ」みたいな感じでなんとなく納得しちゃうみたいなことが普通にありえます。
そういうときに、なぜこのコメントをしたかという理由が添えられていると「でもこれは〇〇なのでここでは大丈夫です」とか「〇〇だったらこういうやり方もありますよ」みたいなコミュニケーションが取れて、レビュイーレビュアー双方が納得してレビューを進めることができるようになると思います。
褒めるときもなぜGOODなのかの理由を添えてあげるとレビュイーも嬉しいと思います。これも必ずしもそのイケてる実装を意図して書いているとは限らないので、理由を添えて褒めるとレビュイーの中でこれはいい実装だったんだなという学びになると思います。
でも、コードにただ👍って一個絵文字が添えてあるだけみたいなのもクールでかっこいいので、そこはいい感じに使い分けていきましょう。
あと、レビュイーは先に指摘がきそうな実装部分に「〇〇という意図でこの実装にしました」みたいなことを書くとレビューの手助けにもなると思います。

コメントは放置しないようにしよう

これはレビュイー・レビュアー共にですね。
せっかく時間を割いてコメントを書いてくれているので、コメントは放置しないようにしましょう。
返信に時間がかかりそうなときは、とりあえず調査に着手してますとかの報告入れたりとか、👀のリアクション付けたりとかで、放置はしてないよってことをちゃんとアピールしておきましょう。
質問（ASK）とかは会話の終わりが分からなくなりがちなので、質問者は納得したときに👍みたいな会話の終わりを認識できる感じのリアクション付けたりしていくといいと思います。
あと、レビューの結果手直しが必要そうだなになったときは、修正したときのコミットへのリンクを該当の指摘があったコメントのスレッドに貼り付けて「ここの作業で修正完了しました」みたいなのを共有するとレビューが捗ると思います。

LGTMしよう

LGTMはLooks Good To Meの略で「僕はいいと思います」の意味ですね。
とりあえずapproveしてレビューおしまいでもいいんですけど、せっかくなのでLGTMコメントで締めるといい感じにレビューの終わりも分かりやすくていいと思います。
僕はプルリクエストにこれを書いてapproveするのが夢だったので初めてメンバーのプルリクエストに「LGTMです！」って書いてapproveした時は小躍りしました。
ちなみにここは（ビジネスの範疇で）いくらでも遊んでいい風潮があるのでChrome拡張のLTTMとかLGTM用の画像生成サイトのLGTMoonとか使って遊ぶとみんな笑顔になれていいと思います。

絵文字使おう

これは完全に個人的な意見なんですけど、単純にコメントが無機質に見えちゃうのを緩和するためにという意図です。

[GOOD]
いいコードですね

より

[GOOD]
いいコードですね👍

の方が嬉しいし、

[COMMENT]
ここイケてないんで何とかしたいですよね

より

[COMMENT]
ここイケてないんで何とかしたいですよね😩

の方がこの表情してるメンバーの顔が浮かんで和みます（少なくとも僕は！）。
僕は絵文字大好きなのであらゆる文章の末尾に絵文字をつけて回りたいんですけど、絵文字が含まれている文章は情報伝達率が落ちる説があるそうなので、使うときは褒めるとき（GOOD）とかコメントするとき（COMMENT）のときとかに絞った方がいいかもです。

おわりに

プルリクエストはちゃんと運用したら品質の面でもチームの文化の面でもいいことがいっぱいあると思うので、プルリクエスト文化がないチームにいる人は頑張って根付かせて欲しいです。
実際にどういう感じで運用したらいいのかとかが分からなかったから前職で導入を強く提案できなかったところがあるので、そういう人たちの助けになれば嬉しいです。