はじめに
皆様こんにちは、株式会社プラハのAwataです。
今日は、以前書いたリーダーの振り返り記事で軽く触れていた、プルリクのレビューについての記事を書いていこうと思います。
なぜプルリクのレビューにルールが必要なのか
それでは早速本題に入りましょう！
チームで開発をしていくうえで、プルリクのレビューという作業が必ず発生します。
そんな中で、以下のような問題が発生したことはないでしょうか？
－溜まり続けるレビュー、そして誰もいなくなった－
Aさん「よし、できた！プルリクのレビュー依頼！よろしくねーっと」
Bさん「うーん、今はちょっと自分の作業に集中したいな、、、。まあ、レビューは終わってからでいいか！」～3時間後～
Aさん「若干今の作業とコンフリクトしそうだから、早くレビューして欲しいんだよなあ、、、。まあでも仕方ないし待つかー。」
Bさん「よし、終わった！プルリクのレビュー依頼出そーっと」
Aさん「うーん、今はちょっと自分の作業に集中したいな、、、。まあ、レビューは終わってからでいいか！」繰り返し...
かなり極端な例を書きましたが、このようにレビューを後回しにしてしまうと、プルリクのレビューがどんどん溜まっていきます。
自分はプルリクのレビュー待ちが溜まると、以下のような問題が発生すると考えています。
- プルリク同士でコンフリクトが発生する可能性が高くなる
- それを恐れて実装を待ってしまったり、コンフリクト解消に時間がかかったりと無駄な時間を使ってしまう
こういった状況を避けるためにも、プルリクのレビューはなるべく早く終わらせてマージするべきで、そのためにはチーム内のルールを作って運用していくことが必要だと考えています。
どんなルールを作ったのか
それでは今回の案件で採用したルールについて紹介します。
ルールつくりの前に、プロジェクトの初期にチーム全体で話し合いの時間を取りました。
すると、プルリクのレビューについて以下のような共通認識を持っていることが判明しました。
- プルリクのレビューを素早く終わらせることは大切だと思っている
- 「1時間ごとに絶対レビューに時間を取りましょう」のようなルールを作ってしまうと、レビューの負荷が高くなりすぎて、作業効率が落ちるのではないか
そんなにうまくはいかない
上記の前提をもとに、以下のようなルールを作ってみました
- 「仕様を満たしているか」と「実装がイケているか」の観点を分けてレビューしたらどうか？
- 仕様を満たしているかのレビューはPRのレビューとして行う
- 実装がイケているかのレビューは原則PRのレビューでは行わない
- 実装がイケているかは別途MTGを開催してそこで話し合う
-
いつレビューをするべきかは明確なルールを作らない
- 仕様を満たしているかを見るだけなら、そこまで時間はかからないだろうという判断
しかし、このルールではうまくいきませんでした。
なぜうまくいかなかったのか
実際に運用したところ、以下のような問題が発生してしまいました。
- 気になるコードを見つけたとしてて、これってレビューで話すことですか？という風に無駄に考えなければいけない状態になった
- 指摘を受けたとして、修正すべきかどうかの判断も難しい
- 別途MTGするならプルリクで話した方が早いのでは？という疑問が生まれた
- **とんでもなく汚いコードがあったとして、仕様を満たしているならそれもマージするのか？**のような葛藤も生まれた
- 仕様を明確に理解している人でないとApproveがしづらい状況になった
- レビュイーもいつマージして良いのか分からない状態になり、全員がApproveするまでマージされないことがあった
- 結果プルリクがマージされるまでの時間は全く短縮されていなかった
そしてリベンジ
前回の反省を踏まえて、以下のようなルールを作り直しました。
- PRは1人がApproveした時点でマージして良い
- マージする人も決めない
- レビュワーが納得するまで議論をして良い
- テキストでやり取りするか、通話でやり取りするかは自由
- レビューしていないメンバーは、マージされた後でも良いので必ず中身を確認する
- 気になることがあれば、たとえマージされていてもコメントして良い
どうなったか
このルールよって、以下のような効果が得られたと感じています。
- 無理に全員がレビューを急ぐ必要がなくなり、気軽にレビューできるようになった
- 「これって指摘して良いのかな？」と迷う必要がなくなり、レビュワーの負荷が下がった
- マージされた後に絶対に見てね、というルールが作られたので、中々レビューできていない人の心理的な負担も下がった
ただし、このルールには、以下のような問題もあります。
- 1人がApproveした時点でマージして良い運用にしたため、Approveしたレビュワー以外がプルリクの内容を知らないまま進んでしまう
これを避けるために、用意したルールがこちらです。
-
マージされたプルリクも必ず見る
- マージされた後に気になることがあれば、コメントして良い
まとめ
今回、自分たちのチームではこちらのルールを採用してうまくレビューが回るようになりました。
自分好みにカスタマイズして、ぜひ自分のチームでも試してみてください！
最後に参考までに使っていたプルリクのテンプレートをのせておきます！
レビューのルールについては他のドキュメントにまとめていたので記載されていませんが、ご自由に使ってください！
PRテンプレート
## チケットもしくは概要
<!--
close #XX
or
ログイン機能の実装
-->
## やったこと
<!--
- Hoge コンポーネントに props を追加
- Fuga コンポーネントを実装
-->
## 特に見て欲しいところ
<!--
- 新しいライブラリを追加したが、他に妥当な物があるか
- テストケースがこれで十分かどうか
-->
## 動作確認手順
<!--
1. ログインフォーム(/login)にアクセス
1. メアドとパスワードを入力してログインを実行する
1. ログインが成功してユーザー一覧ページにリダイレクトされること
-->
## TODO（できればIssueを立ててリンクを貼りましょう）
<!--
- [ ] バリデーション
- [ ] エラーメッセージ
-->
## その他情報
<!--
参考にした記事
-->
## レビューコメント
レビュー時は prefix（テキストでも絵文字でもOK） を付けて、どのレベル感のコメントか明示してください
- ⚠️ must: 必ず直すべき
- 💭 imo: 自分の考えは〜
- 🐾 nits: 細かい指摘
- ❓ ask: 質問/確認
- 🖊️ memo: メモ書き
## マージについて
マージするのはレビュワーでもPRを開いた人でもどちらでもOKです！
Approveはするけどコメントを残すね→開いた人が修正するかどうか判断し、そのままor修正後にマージ
完璧なPRだ！→レビュワーがそのままマージ
Discussion