Pull Request まわりの考察
Pull Request (PR) におけるレビューの成熟度
これは完全に個人的な私見であり、レビューの成熟度という言葉を使うのは不適当かもしれない。
■ 概要
| レビューの成熟度 | レビューでなされる指摘内容 |
|---|---|
| 1 （低い） | 静的解析で検知し得る不備 |
| 2 | 実行時エラーで検知し得る不備／一部のソフトウェア品質を損なう不備 |
| 3 | ある１ファイルのみで判断できる、リポジトリの一貫性を損なう不備 |
| 4 | 複数ファイルによって判断できる、リポジトリの一貫性を損なう不備 |
| 5 | PR の実装のみで判断できる、実装されたロジックと要求仕様との不整合 |
| 6 （高い） | PR の実装によって既存のコードに発生する、要求仕様との不整合 |
■ 付記
注意点として、高い成熟度を目指すことが必ずしも望ましいとは限らない。
高い成熟度を目指すには、以下が必要になると思われる；
- lint といった静的解析をするツールや、テストを実行するツール
- リポジトリに関するドキュメントや、それを保守するフロー
- PR を作成するレビュイの／PR を読むレビュワの時間・体力
- 低い成熟度のレビューでも構わないのならば、PR を作成する／読む時間・体力は、比較的小さいものになるであろう
- PR を適度な粒度で作成することは、レビューの成熟度の高さに寄与するであろう（この詳説は次の節に譲る）
- PR を適度な粒度で作成しないと、チームとして目指せる現実的なレビューの成熟度の限界は 3, よくて 4 程度であるように感じる
- 詳細な要求仕様と、それを管理するためのツール
これはトレードオフとも解釈でき、チームとしてレビューにどこまでを求めるかという判断でもある。レビューの成熟度が低いと、それぞれの領域ごとに属人化が進むであろう。ただ、少なくとも短期的には開発速度が比較的期待できるであろう。
開発初期だったり、メンバーが 2, 3 人程度と少なかったりするときは、レビューの成熟度が低いことをより許容し得るかもしれない。
1. 静的解析で検知し得る不備
指摘の概要
typo や line wrap, スペーシングといった lint のようなツールを通せば検知できるもの。
レビュワに求められるもの
なし（しいて言えば可読性の知識）。
チームに参加してすぐのひとでも指摘できる。
補うためのもの
lint のような 静的解析 のツールを導入して 継続的インテグレーション (CI) でまわす。
付記
ルールがない状況下で手を加えようとしても、一貫性は向上しないし、編集合戦になってしまう可能性がある。ルールがなく強制もできないのならば、いっそ放置していたほうがまだマシかもしれない。
2. 実行時エラーで検知し得る不備／一部のソフトウェア品質を損なう不備
指摘の概要
動作確認やテストを網羅的に記述していれば検知できるもの。
また ソフトウェア品質 を損なうものとして、 不必要に計算量の大きい処理といったパフォーマンスに悪影響を及ぼすもの、 SQLインジェクション や ディレクトリトラバーサル といったセキュリティ上問題があるものなどが考えられる。
レビュワに求められるもの
使用している言語や FW, ライブラリなどへの知識。チームに参加してすぐのひとでも、それらへの知識さえあれば指摘できる。
補うためのもの
テストを導入し、適当に記述して CI でまわす。レビュイにはテストを適当に記述する技能が求められる。
なんらかの事情でテストが導入されていないのならば、処理を実行して結果を転記することも考えられる。たとえば Web バックエンドであれば、実際にリクエストしてみて結果を転記するなど。
3. ある１ファイルのみで判断できる、リポジトリの一貫性を損なう不備
指摘の概要
チームで決められた、命名などのコーディング規約の違反しているもの。すでにある機能に気付かず、再実装しようとしているもの（ 車輪の再発明 ）など。
あわせて、リポジトリの一貫性を（チーム内での合意を）考慮しながら行う必要がある、ある１ファイルにおける リファクタリング もここに含んで良いであろう。
（リファクタリング については 『リファクタリング』 などを参照されたい）
レビュワに求められるもの
チームで決められたコーディング規約や、既存実装への表層的な知識。
補うためのもの
コーディング規約や、既存実装に関するドキュメントを整備する。
4. 複数ファイルによって判断できる、リポジトリの一貫性を損なう不備
指摘の概要
ソフトウェアアーキテクチャ レベルにおいて、既存のコードとの一貫性が損なわれるもの。ないしチーム内での合意に反しているもの。複数ファイルにまたがる先述したようなリファクタリングも、ここに含んで良いであろう。
（ただし要求仕様には影響しないレベルのもの）
レビュワに求められるもの
そのリポジトリのアーキテクチャへの知識。
補うためのもの
リポジトリのアーキテクチャに関するドキュメントを整備する。レビュイは UML といったダイアグラムを記述して、不備を判断しやすくするのも良いであろう。
（UML については 『UML モデリングのエッセンス』 などを参照されたい）
5. PR の実装のみで判断できる、実装されたロジックと要求仕様との不整合
指摘の概要
実装されたロジックそのものは処理として妥当だが、 要求仕様 を満たしていないもの。
あわせて、たとえば、PR に記述された SQL のクエリによって fetch される行数は、要求仕様を考慮すると多大になるであろうため、取得条件や周辺の処理を変更するよう提案する、などといった指摘もここに含んで良いであろう。
レビュワに求められるもの
レビュイが本来どのような機能を実装しようとしているのか（つまり要求仕様）の把握と、それをレビュイが実装したロジックが満たしていないことを判断する技能。
補うためのもの
要求仕様がまとめられたドキュメント（へのアクセス）。
ClickUp や wrike といった『タスクを管理するシステム』（ バグ管理システム など）で切られたチケットや GitHub の issue などと、その PR が対応関係にあるような運用を行うと、要件としての把握が容易になると考えられる。
TDD, BDD, ATDD といった開発手法を導入しており、テストなどが要求仕様を満たすように記述されていれば、それをパスしたかによって、要求仕様との整合性を判断することもできるであろう。
（『テスト駆動開発』 、『The BDD Books - Discovery』 などを参照されたい）
レビュイは実現したい要求仕様の理解を、ソースコードのドキュメントコメントに記述するのも良いであろう。また、たとえば Web フロントエンドにおいて画面を実装しているのならば、そのスクリーンショットやそのアニメーションを転載するのも良いであろう。PR を確認するための開発環境を用意することができれば、更に判断がしやすくなる（コストが大きいため難しいことが多いが）。
付記
レビュワに高い成熟度でレビューを行なってもらうために、PR で実現したい要求仕様は、コード以外の方法でも詳細に表現されているべきである。
成熟度の低いレビューが常態化しているチームだと、PR のレビューが通っているにも関わらず、あの領域は実装者しかわからないといった状況に陥ってしまう可能性が高くなるであろう。
また、要件管理の品質を保つためにも、同じ内容が複数の箇所に書いてあることは望ましくない。
（これは 信頼できる唯一の情報源 と呼ばれる）
たとえば「実現したい要求仕様については『タスクを管理するシステム』のチケットにすべて書いてあるべきで、技術的な留意点などについてはソースコードに書いてあるべき」だとすると、PR のコメントに書かれる内容は、そのいずれにも該当しないものでなければならない。
6. PR の実装によって既存のコードに発生する、要求仕様との不整合
指摘の概要
実装されたロジックそのものは処理として妥当であり要求仕様を満たしているが、その実装によって、既存の実装が要求仕様を満たさなくなるもの。
換言すると、レビュイが見落としていた、PR に含まれていない既存実装への変更。
レビュワに求められるもの
上掲したものに加えて、既存のコードにはどのような要求仕様があり、どう実装されているかを把握していなければならない。
補うためのもの
上掲したものが、既存のコードにおいても適切に実施されていること。
付記
大きな・複雑な機能を複数人で実装しているときは特に、すべてのチームメンバーがその機能全体の要求仕様を十分に理解している、という状況を実現することが困難になるであろう。
PR は、チームメンバー間における要求仕様に対する理解の水準を、均一に近付けることができる重要な機会であると考える。そういった意識をチームメンバーが明確に抱きながら開発を続ければ、属人性を少なくすることができるはずである。
適度な粒度の PR にする
■ 適度な粒度とは／なぜ適度な粒度にするのか
... if we want a 90% chance of completing a code review in an hour, we probably need to cap our pull requests at around 200 lines of code.
1 時間以内にレビューが終わる確率を 90% にするためには、PR の上限を 200 行程度にする必要があろう。
... a review of 200-400 LOC over 60 to 90 minutes should yield 70-90% defect discovery.
With this number in mind, a good pull request should not have more than 250 lines of code changed.
60 - 90 分費やして 200 - 400 行のコードをレビューすれば、70 - 90% の不具合を発見できるはずである。
この数字を念頭に置くと、良い PR は、コードの変更を 250 行以下にすべきであろう。
（孫引きとなってしまっているが、おめこぼしいただきたい。なお 250 行という値は、引用記事内の図が意識されている）
...
PR の粒度が大きいと、それだけレビュワの負担が大きくなり、結果レビューの質が落ちる。
見なければいけない内容が多いと、それに費やす時間は大きくなるが、集中力の持続には限界があるためである。また外部の都合によって、レビューを中断しなければいけない可能性も増えてしまう。
どの程度を適当な粒度とするかは、チーム内で合意を得るのが望ましいであろう。個人的には diff の +, - の合計が 400 を超えると大きいと感じる。
この記事で挙げている行数という判断基準は、あくまで一例であり目安である。たとえば、この基準を満たすために行数を減らそうとするなどといったことは、本末転倒であり行ってはならない。
■ どうすれば適度な粒度の PR を作成できるのか
以下 3 つの方法が考えられる。
1. あらかじめ十分に精度の良い設計をしておき、コーディングする前から PR の粒度がいくらか予想できるくらいにしておく
- これが正攻法であるように思われる
- その設計をチームメンバーでレビューできたら、更に望ましい
- 事前にチーム内で合意がなされているため、手戻りがあっても実装者のみが責任を感じる必要がなくなる
- 設計の構造が頭に入った状態でレビューできるため、負荷が減り成熟度も高めやすくなる
- あらかじめの設計に時間を費やさなければならない
- 具体的になにをどこまでやるのかということは、まだ語れるほど自分の中に何かがあるわけではないので、ここでは触れないでおく
- 少なくとも、やりすぎるべきではないように感じる
- 具体的になにをどこまでやるのかということは、まだ語れるほど自分の中に何かがあるわけではないので、ここでは触れないでおく
- 予想された PR の粒度が大きいのであれば、それは PR が担おうとしているタスクの粒度が大きい可能性がある
- タスクが分割可能ならば分割する
- 分割しないほうが望ましそうであれば、その PR が粒度の大きいものになることを許容しても良いであろう
2. 全部つくってから、適度な粒度に分割する
- 分割する手間がかなり大きい
- CI でテストを回しているならば、全体ができてからテストが通る単位で分割するのは、容易でないことがある
- たとえば特定パターンのブランチ名は CI のテストをスキップする設定をしておき、feature ブランチ からその特定パターンのブランチを切って分割された PR を投げるを繰り返し、最終的に feature を develop にマージする PR でテストの CI を回す、という戦略も考えられる
- 本来不要な、一時的な記述を追加しなければならないことがある
- 複数の PR に分割したあと、早い段階で比較的大きな修正が必要になったとき、後続 PR すべてに影響してしまう
- CI でテストを回しているならば、全体ができてからテストが通る単位で分割するのは、容易でないことがある
- レビューの待ち時間がボトルネックになり得る
3. 事前準備なく、適度な粒度で PR を作成する
- 一度レビューしてもらったあと、軌道修正によって、レビュー済みの部分に変更が入る可能性が比較的大きい
- 変更前の部分をコーディングする／レビューしてもらう時間・体力が浪費されてしまった結果となる
- 実装者がつよつよで、事前準備なくとも、軌道修正不要な PR が作成されるかもしれない
- 未来永劫チームメンバーみんなが常にそうであるならば、個々の信念に委ねれば良いであろう
■ ルールがないとき、なぜ粒度は大きくなるのか
以下 5 つの理由が考えられる。
1. レビュイが粒度の小さい PR を作成する必要性を感じていない
- あるいは必要性を感じているものの、適度な粒度に分割する作業には見合わないと感じている
- これは正直なところ、個々人の考え方に基づく部分もあり、強制しづらい
- レビューにどこまでの指摘を（つまり先述したレビューの成熟度を）求めるか
- どの程度を超えれば PR が大きいと感じるか
- たとえば diff の
+,-の合計が、どの程度の値を超えれば PR が大きいと感じるか、という体感によるということ- 1000 を超えれば大きいと感じるひとが、400 での分割を行うのは、余計な手間であるように思われるかもしれない
- たとえば diff の
2. 頻繁にレビューを依頼することに対して、負い目のようなものを感じている
- レビューの依頼（やレビュワの指定）を自動化するような仕組みを実装すれば緩和できる
3. レビュー待ちという状態を（たとえばデプロイの）ボトルネックにしたくないと考えている
- 複数の PR になると、それだけレビューのタイミングが増えることになるため、レビュー待ちという状態が発生しやすくなるかもしれない
- 2., 3. については、PR のレビューをする時間を定常的にとるなどといった対策で緩和できる。また PR が放置されがちであるのならば、レビュワを抽選して指名する運用を検討するのも良いであろう
4. 記述の方法に原因があり、分割できない
- 設計の段階から既に問題があるように思われる。PR を作成する前に（あるいはもっと以前に）相談したほうが良いと思われる
5. コミット単位で適度な粒度に分割されていれば、PR は大きくても良いと考えている
- 対応関係にあるタスクと PR とを、原則一対一で紐付けるべきであると考えているのかもしれない
- メリット／デメリットがあるので、これについてはチーム内での話し合いが必要かもしれない
- レビューされるタイミングが 1 回のみである
- pros: レビューがボトルネックになり、レビュイが進行できなくなる／レビュワが高い優先度でレビューを求められる機会が減る
- レビュワは、コンテキストスイッチの機会を 1 回に抑え得る
- まとめてレビューできるため、前後の関係を参照しやすい
- cons: 設計上の不備や要求仕様への誤解などがあったとき、それが正されないまま作業が進んでしまう可能性がある
- cons: 作業の進捗度合いが比較的判断しづらくなる
- cons: 他メンバーが作業中の内容にも影響を及ぼすものであるとき、その影響は PR が投げられるまで発覚しないかもしれない
- pros: レビューがボトルネックになり、レビュイが進行できなくなる／レビュワが高い優先度でレビューを求められる機会が減る
- cons: 修正が発生したとき、コミットを積み上げるか、発生した個所からの rebase が必要になる
- 時系列にコミットをレビューしていったとき、指摘箇所を見つけても、後続のコミットで修正されている可能性がある
- レビューされるタイミングが 1 回のみである
- どちらか一方を常に選択し続ける必要性はなく、一方を主流として一方を消極的な選択肢とするなどが良いであろう
粒度の小さい PR を作成する必要性を感じていないレビュイについて
もう少し 1. について詳しく考えてみると、粒度の大きい PR を作成するレビュイは、次のいずれかに同意すると考えられる。
-
成熟度の高いレビューをレビュワに求めていない
- 自身がレビュワにアサインされたとき、成熟度の高いレビューをしようとも考えていないであろう
-
粒度が大きい PR に対して、成熟度の高いレビューを行うことは、自身にとってもメンバーにとっても容易であると考えている
- 自身がレビュワにアサインされたときも、成熟度の高いレビューをしようとするであろう
- 成熟度の低いレビューばかりをするメンバーに対して、未熟さか怠惰さを覚えるであろう
どの程度の成熟度をレビューに求めているか、チームとして合意がなされていないのならば、それは個々に委ねるしかなく個々の自由である。
前者は成熟度の高いレビューを求めていない。後者は成熟度の高いレビューを求めてはいるが、PR の粒度は（少なくともそのひとが作成する PR の大きさまでは）成熟度に深刻な影響を及ぼさないと考えている（一方で、後者のレビュワは粒度の大きい PR に対して、成熟度の高いレビューは求めていないのであろうと解釈するかもしれない）。
もしあなたが PR の粒度が大きすぎると感じているのならば、きっとあなたはレビュワとして成熟度の高いレビューをしようとしており、それには PR の粒度が（後者のひとが感じるより小さい閾値で）影響すると考えているのであろう（それ以前に、先述した「補うためのもの」のような仕組みが不足しているのかもしれない）。
前者については、求めるレビューの成熟度をチーム内で決めることによって、解消できるであろう。あるいは、レビュイが PR ごとレビュワに求めるレビューの成熟度を明示する運用を行うことも考えられる。後者については、どの程度の粒度からレビューの成熟度に深刻な影響を与えるのか、チーム内で話し合い合意された目安を設けるべきであろう。前者後者いずれかであるかを区別して考える必要性はないように思えるから、両方を行うのも良いであろう。
PR の粒度が大きくなればなるほど、より複雑さも大きくなると考えるのが自然である。 また先述したように、レビューの成熟度が高くなるほど、より複雑なものがレビュワに求められる。
複雑なものに対して、複雑な操作を行うことは本質的に困難であると思われるが、どの程度困難であると感じるかには個人差がある。他者がどの程度困難であると感じているかは、話し合い聞き出してみないと判断が難しいであろう。
■ 1 つの PR では、なるべく 1 つのことをやる
1 つの PR で複数のことをやっているのならば、まず分割できるはずであろう。
複数のことがなされている PR は、レビュワにとっても負担が大きいだけではない。それぞれの関心ごとで議論が発生したとき、コメントが入り乱れてしまい分かりづらくなってしまう。
また問題の切り分けが困難になってしまう可能性も考えられる。たとえば PR にあるすべてのコミットを revert すれば済むはずが、PR にあるそれぞれのコミットを精査して、revert を各々指定しなければならないという事態も発生し得る。
個人的には、複数のことをやっていて個々に分割可能ならば、結果どんなに小さな diff になろうと原則分割という方針が望ましい。
特にリファクタリングについては、つい混ぜ入れてしまいがちなので、強い心を持って分割したほうが良い。
（それを受け入れやすくするために、リファクタリングに対して寛容な姿勢や仕組みが必要であるかもしれない）
■ 分割の方針について
ある用語の名称が変更になったため一括で置換する、自動生成されたファイルを追加する、といった機械的な変更は、独立した PR になっていることが望ましい。
その上で、機械的な変更は PR の粒度よりも、作業単位を優先して PR を作成したほうが良いであろう。更なる分割をしてしまうと、かえって確認が困難になり得るためである。
（ファイルを生成してそれを反映する PR を作成する、というところまで、自動化されていると尚良いであろう）
+, - いずれもある PR よりも、+ or - だけである PR のほうが、たいていレビューしやすいと思われる。
+ or - だけの（ないし、一方に大きく偏る）PR に分割できるほうが、設計としても好ましいことが多いであろう。そのような PR は、モジュール性が良い（高凝集・低結合である）、OCP を守れている傾向にあると考えられるためである。
（モジュール性は 『ソフトウェアアーキテクチャの基礎』 、OCP は 『Clean Architecture』 などを参照されたい）
暫定的な実装に立ち向かう
■ 放置された暫定的な実装；割れ窓をつくらない
あとでリファクタリングをするだとか、あとでテスト書くだとか、そういう動作確認を最優先して暫定的な実装をすることは、場合によっては求められることである。現実的には、排除することは難しいであろう。
暫定的な実装に対して、丁寧なレビューするモチベーションが湧かないことは確かである。しかし、その暫定的な実装のうち、おそらく一部は忘れられたり放置されたりで、そのまま問題がないと見逃され master / main にマージされ得るであろう。
すなわち暫定的な実装に対して、丁寧なレビューをして暫定的な実装であろうと質を求めるか、忘れられたり放置されたりを必ず潰す仕組みを用意するかしなければならない。
放置された暫定的な実装のような質の低いコードが混じると、プロジェクト全体として質が低いコードが許容されているのだと認識され、徐々に質の高いコードの割合が低くなっていく可能性が考えられる。
（これは 割れ窓理論 と呼ばれる。 『達人プログラマー』 にも記述があるので参照されたい）
■ 使い捨てのプロトタイプを活用する
暫定的な実装ということは、その後にあらためて暫定的じゃない実装に上書きされるということである。繰り返しになるが +, - いずれもある PR よりも、+ or - だけである PR のほうが、たいていレビューしやすいであろう。
いっそそれならば使い捨てのプロトタイプということにしてしまい（ プロトタイピング ）、暫定実装はレビューなしで（最小限で）開発を進める。のちに暫定実装はすべて破棄して、あらためて暫定的ではない実装をレビューしてもらったほうが良いかもしれない。
疎結合なモジュールとして設計されているのならば、不確定要素は テストダブル のようにロジックを記述せずハードコーディングしてしまい、不確実性の低い実装部分のみレビューをもらう運用にする、といったことも考えられる。
PR の品質を高くする
■ CI で検出できる範囲を増やす
CI で静的解析・ビルド・テストなどを行うことで、レビュワに指摘される以前にレビュイが修正できる範囲を増やすことができる。
typo もたとえば Node.js ならば textlint で検出することが可能である。
git のコマンドに hook させて 、remote に push する前に走査を強制させることなどもできる。
（Node.js ならば husky など）
■ 可読性の高い PR を作成する
可読性の高いコードの記述手法については、多くの支持を集めている文献が存在している（ 『リーダブルコード』 、『Clean Code』 など）。チーム内での合意として明文化されていない部分については、それにならうのが望ましいであろう。たとえば、「こうしたほうが良いと思われるから変更してほしい」と指摘するより、「こうしたほうが良いと『リーダブルコード』に書いてあったから変更してほしい」と指摘したほうが受け入れられやすいと考えられる。
個人的には、可読性について PR で指摘を受けるのは、もったいないように感じる。可読性の高いコードの記述手法は、設計手法などと異なり、より具体的で機械的なものであるように思われるためである。
雑多な Tips
■ PR は IDE からのほうが見やすそう
PR の diff やコメントは、IDE の標準機能ないしプラグインを利用すれば、IDE から確認できる。ブラウザから確認するよりは見やすいであろう。
たとえば JetBrains ならば
- GitHub: https://www.jetbrains.com/help/idea/review-code.html
- GitLab: https://plugins.jetbrains.com/plugin/18689-gitlab-merge-requests
■ diff の whitespace を無視して表示できる
?w=1 を付ければ、どの diff の画面であろうと whitespace を無視できるはず。
GitHub
GitLab
■ Mermaid でダイアグラムを描画できる
GitHub
GitLab
■ Markdown で折り畳みを使える
■ Markdown でタスクリストをつくれる
GitHub
タスクリストをすべて checked にしないと merge できないようにも設定できる。
GitLab
■ レビュワを自動抽選できる
GitHub
GitLab
There are some options that are similar to auto-assign but require Premium or Ultimate plans (they're not available for free plans).
■ Draft Pull Request を使える
GitHub
GitLab
■ 説明部分にテンプレートを用意できる
GitHub
返信コメントにもテンプレートを用意できる
GitLab
■ コミットメッセージにもルールを設定できる
ルールとしては Conventional Commits が有名。
それに準拠させるツールもある。たとえば https://commitlint.js.org/
Discussion