僕の考える、GitHubでチーム開発をする際のプルリクエスト（プルリク）の作法を書いてみました。プルリクという名前から当たり前ですがGitHubを使うことを想定してます。
十分小さくプルリクを作ろう
プルリクが小さいと、レビューが簡単になり、変更がすばやく中央のブランチに取り込まれるため、レビューの精度が高くなり開発スピードも早くなります。
タスクやIssueが一つで表現されていても、やりたいことを予めいくつかに分けて、それに対して一つプルリクを出しましょう。コードレビュー中に他にやりたいことややるべきことが出てきたら、同じプルリクで作業せずに別のプルリクで行いましょう。
プルリクを出す前に「これとこれとこれのプルリクを出します」とIssueなどで宣言されている状態がベストです。プルリクの分け方に不安があるならこの状態でもレビューをもらっておくと良いでしょう。
単体でリリースできる単位でプルリクが出せると、すぐにリリースしてユーザーに悪影響を与えない安心感を得られるので最高です。アプリに機能トグルの機能があるとさらに楽になります。
プルリクエストに必要な情報を入れよう
書いたコードだけでなく、プルリクも大事なアウトプットです。このプルリクエストが何を実現したいためのプルリクエストなのか、レビュワーは何をレビューすればよいのか、このあたりがわかるのが大事です。
本文には以下のようなことをMarkdownの見出しにして書くと便利です。
- 目的
- 達成条件
- 実装の概要
- レビューして欲しいところ
- ここはこうしたけどこの点で問題はないだろうか
- このあたりいじってるけど特に悪いことはないか
- 不安に思っていること
- 何をレビューしてほしいかを書こう
- 設計や企画のレビュー
- 保留してること
- このプルリクでしないこと
- レビューの範囲を絞れる
- スケジュール
- マージすべき日、リリースすべき日の指定があれば書く
- 関連
- 関係するプルリクエスト
必ずしも全てを書く必要はありません。
プルリクエストのテンプレートをチームで予め作っておくと便利です。
Creating a pull request template for your repository - User Documentation
また大切なのが、他の関連あるissueやプルリクへのリンク#(プルリク番号) をすることです。バグの修正ならバグを生んだプルリクをリンクしたり、機能の中身について議論したIssueがあればそちらを参照するなどしましょう。リンクされたissueやプルリクの方でもリンクされたことが確認できるようになり、相互リンクになります。
asigneeとreviewerを指定しよう
プルリクを見てレビューしてほしい人を指定します。
asigneeとreviewerの使い分けには諸説ありますが、僕は、asigneeが最終的なマージしてよいという判断をしてマージボタンを押す人、reviewerが変更の詳細に対してレビューする人、のような区別をしています。ただ役割がかぶることも多いので、同じ人をどちらにも追加したり、reviewerしか使わないようなことも多いです。ここはチームによってやり方が異なると思います。
早めにプルリクを出そう
プルリクを小さく分けることと似ていますが、できたところから早いうちにプルリクを出しましょう。もし一つのやりたいことに対して数日間プルリクが出せないなら、一つのプルリクでやろうとしていることが大きすぎる可能性があります。
実装方針に迷っていたり、実装途中で随時細かくレビューがほしいなど、どうしても時間がかかってしまう場合は、Draft PRを出したり [WIP] をタイトルに付けたの形にして未完成の状態でプルリクを出すこともあります。プルリクをDraftやWIPにする際は、本文に「このプルリクはいつWIPが外れるのか（review readyになるタイミング）」を必ず明記しましょう。
status checkの失敗やコンフリクトはプルリクを出した人が早めに直そう
今どきの開発チームならCIが連携されていて、プルリクごとにテストが走るようになっているはずです。テストがコケている場合は、マージできないことは明らかなので、プルリクを出した人が早めに修正をpushしましょう。
コンフリクトがおきている場合もそのままマージができないので、マージ先ブランチをトピックブランチにマージして先にコンフリクトを直してpushしておきましょう。
また事前にbranch protection ruleで必須のstatusを指定しておくのがオススメです。
レビュワーやasigneeはすぐにプルリクを見よう
プルリクが細ければ細かいほど、プルリクの数も増え、なるべく早くレビューしてマージを目指すことが大切になります。前のプルリクがマージされないと次の作業に取りかかれない、ということもあります。
例えば、asigneeやreviewerに指定されてから1日（1営業日）以内には見るようになど、ある程度返事をする目安を決めておくとスピード感が出ます。この場合は、1日以内にレビューができなさそうであれば、いつごろレビューするというコメントを早めに付けておくとよいでしょう。
またレビュワーが、自分がレビューすべきプルリクができたことを知るのも大切です。
メールによる通知や、slack integrationによる通知、https://github.com/pulls や https://github.com/notifications など、通知を受ける方法はたくさんあります。
レビューを受けた人も、レビューコメントがついたらなるべく早く確認しましょう。上記のことと被りますが、プルリクを出す人は、見てどこをレビューしてほしいかがすぐわかる程度に変更の目的などを明確に本文に書くようにしましょう。
自分がasigneeやreviewerに指定されていないプルリクも積極的にレビューしよう
asigneeやreviewerだけしかレビューしてはいけない理由はありません。「FF外から失礼します」的な挨拶も要りません。
あなたしか気づいていない実装の問題点や、誰もよくわかっていないけどあなただけ疑問として浮かんだことがあるかもしれません。自分の担当のプロジェクトに対しては、どんなプルリクが出ているかだけは常にwatchしておくとよいです。
コードにレビューコメントをつけよう
レビューによって、プルリクがマージしてよい状態であることを保証し、最終的にマージを行います。
これにはGitHubのCode Review機能を利用します。Files changedタブを押して、問題点の指摘や、疑問点の解消、よりよい実装の提案、意図の確認などをコメントとしてつけましょう。「こういう書き方できるんだ、知らなかった！」などもぜひコメントにつけましょう。
一通りコメントを付けたら、右上の Review Changesでレビュー全体のコメントを付けて、 Submit review します。単に質問やコメントを残したい場合はComment、問題ないと思ったらApprove、問題があるのでマージできないときはRequest changesを選択しましょう。
このようなやり取りを複数回重ねて、statusチェックも通って、approveが複数人からついたらマージ、というのがよくある流れです。
積極的にプルリクを閉じよう
以下のような場合には積極的に閉じましょう。
- 変更が大きすぎてレビューに時間がかかりすぎるとき
- 小さく分けることで全体のレビュー時間を減らすことができます
- レビューの結果、実装がほぼ作り直しになるとき
- 変更ですまそうとすると逆に手間になることも多いです
- コミットのログが荒れているとき
- 既にマージされているコミットが含まれていたり、余計な変更が入っていたりする時
- ブランチを作り直してもらいましょう
- rebaseしたりsquashしてforce pushするとコミットログは綺麗になりますが、レビューの過程が追いづらくなるので使い所には注意しましょう
- 状況が変わり、変更が不要になったとき
- マージする理由がなくなったらそのプルリクは不要です
- 長い時間進行が止まっているもの
- ブロッカーが消える見込みがしばらく立たないときは閉じて、またレビューできるようになったときにreopenしましょう
プルリクを閉じるときにはなぜ閉じるのかを同時にコメントして、今後このプルリクがどうなるべきか、どうしていくつもりかをプルリクのauthorにも他の人にもわかるように書きましょう。
今後必要になる可能性が高いが、いまはマージできない、という場合も閉じることがありますが、この場合はブランチが不要なものとみなされて削除されないよう注意しましょう。
マージされたら、ブランチを削除しよう
マージされるとDelete branch ボタンが出現するので、特に残す意図がなければマージした人がブランチを消しましょう。
マージしたブランチを自動で削除する機能を有効化しておくと手間が減り便利です。
https://help.github.com/en/articles/managing-the-automatic-deletion-of-branches
GitHubだとブランチごとに関連するプルリクがマージされたかどうか見られるので、残っていないか見てみましょう。
こんな感じ https://github.com/ikuwow/query_ok/branches
アンチパターン
ここまで挙げたものの逆をアンチパターンとして挙げると以下のようになります。
- 全部実装してからレビューを投げる
- 全部をいっぺんにレビューしなきゃいけないので大変です
- 根本が間違っていたら全部やり直しになります
- レビュー後の修正を見ると、コードが全部書き変わっている
- 別のブランチにコミットし直したほうが楽です
- レビュー後にrebaseをしてしまってログが壊れているのも困ります
- レビューが1週間後
- PRが大きすぎたり情報が少なくてレビューしづらい可能性があります
- チームが日常的にレビューリクエストを見る習慣が根付いていないのかもしれません
- 「プルリク出したのでレビューおねがいします」というコミュニケーションが頻繁に行われている
- チームがPRを見る習慣が根付いていないのかもしれません
- GitHubに通知の機能は備わっているのできちんと全員みましょう
- Slack等のチャットに通知するのも便利です
- 「テストコケてるので修正お願いします」というコミュニケーションが頻繁に行われている
- プルリク出した人がなるべく確認しましょう
- 1ヶ月間放置されているプルリクがある
- 既にマージされたブランチが数十個残っている
まとめ
良いソフトウェアを作るために、小さくわかりやすいプルリクをサクサク出して、みんなでレビューしていこうぜ！
雑に書いてしまった感があるので、間違いや誤解を生む点などありましたら指摘いただけるとありがたいです。
その他（書いてないことなど）
- branchの命名規則の重要度は低いです。PRを小さく出す運用をする場合はブランチの寿命が短いためです。
- コミットログをある程度綺麗に保つことも大事です。コミットログの綺麗さをあまり問わない場合はsquash and mergeを使うと便利です