Edited at

チーム開発におけるプルリクの作法

僕の考える、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にしたり [WIP] をタイトルに付けたの形にして未完成の状態でプルリクを出すこともあります。プルリクをDraftやWIPにする際は、本文に「このプルリクはいつWIPが外れるのか(review readyになるタイミング)」を必ず明記しましょう。


status checkやコンフリクトはプルリクを出した人が早めに治そう

今どきの開発チームならCIが連携されていて、プルリクごとにテストが走るようになっているはずです。テストがコケている場合は、マージできないことは明らかなので、プルリクを出した人が早めに修正をpushしましょう。

コンフリクトがおきている場合もそのままマージができないので、マージ先ブランチをトピックブランチにマージして先にコンフリクトを直してpushしておきましょう。


レビュワーやasigneeはすぐにプルリクを見よう

プルリクが細ければ細かいほど、プルリクの数も増え、なるべく早くレビューしてマージを目指すことが大切になります。前のプルリクがマージされないと次の作業に取りかかれない、ということもあります。

例えば、asigneeやreviewerに指定されてから1日(1営業日)以内には見るようになど、ある程度返事をする目安を決めておくとスピード感が出ます。この場合は、1日以内にレビューができなさそうであれば、いつごろレビューするというコメントを早めに付けておくとよいでしょう。

またレビュワーが、自分がレビューすべきプルリクができたことを知るのも大切です。

メールによる通知や、slack integrationによる通知、https://github.com/pullshttps://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


アンチパターン

ここまで挙げたものの逆をアンチパターンとして挙げると以下のようになります。


  • 全部実装してからレビューを投げる


    • 全部をいっぺんにレビューしなきゃいけないので大変

    • 根本が間違っていたら全部やり直し



  • レビュー後の修正を見ると、コードが全部書き変わっている


    • 別のブランチにコミットし直したほうが楽です



  • レビューが1週間後

  • 「プルリク出したのでレビューおねがいします」というコミュニケーションが頻繁に行われている


    • GitHubに通知の機能は備わっているのできちんと全員みましょう



  • 「テストコケてるので修正お願いします」というコミュニケーションが頻繁に行われている


    • プルリク出した人がなるべく確認しましょう



  • 1ヶ月間放置されているプルリクがある

  • 既にマージされたブランチが数十個残っている


まとめ

良いソフトウェアを作るために、小さくわかりやすいプルリクをサクサク出して、みんなでレビューしていこうぜ!

雑に書いてしまった感があるので、間違いや誤解を生む点などありましたら指摘いただけるとありがたいです。