Nguyen Le Phong

やさしく効果的なエンジニアリング全 5 回中第 3 回

レビューしたくなるプルリクエストの書き方

優れたプルリクエストはレビュアーへの贈り物です:小さく、説明が丁寧で、Yesと言いやすい。レビュー可能なプルリクエストの解剖——サイズ、タイトル、説明、コミット衛生、セルフレビュー——を具体的なビフォー・アフターの例で解説します。

あの光景を知っていますか。火曜日の午後、レビューキューにあるプルリクエスト——タイトルは「fixes」、差分は41ファイルにわたる2,347行、説明はなし、そして陽気な「PTAL」のコメント。心が沈みます。それでも開きます。20分後、同じメソッドを三回読んでも何の問題を解決しているのかわからない、残したコメントはたった一つ:「全体的に良さそうです、この変数名を変えてもいいかも?」理解できなかったコードにゴム印を押してしまいました。

著者は悪意があったわけではありません——おそらく没頭していて、仕事に誇りを持っており、コードを出荷することが仕事の半分でしかないことをつい忘れていただけです。もう半分は、レビューされ、理解され、安全にマージされることです。プルリクエストはマージ前にクリアする形式的な手続きではありません;それは会話であり、あらゆる会話と同様に、誰かが理解してもらうための努力をすると、うまくいきます。

この記事はその努力についてのものです:チームメンバーが本当にレビューを楽しめるプルリクエストの書き方——小さく、目的があり、わかりやすく、Yesと言いやすいもの。

プルリクエストはコードではなく、コミュニケーションだ

差分の一行も入る前に、プルリクエストは別の人間へのメッセージです。その人間は限られた時間を持ち、午後11時にあなたが何を考えていたかについて限られたコンテキストを持ち、注目を競う十数個の他のことがあります。著者としてのあなたの仕事は、彼らの仕事を簡単にすることです。

レビュアーを裁判官ではなく聴衆と考えてください。彼らはあなたを捕まえるためにいるのではなく——あなたが堅実なものを出荷するのを助ける協力者です。変更の背後にあるなぜを理解してもらうほど、レビューは速くなり質が上がります。優れたプルリクエストの説明はコードへのカバーレターのようなものです:場面を設定し、動機を説明し、レビュアーに何を見るべきかを正確に伝えます。

このマインドセットの転換がすべてを変えます。プルリクエストを自分のために書くのをやめてレビュアーのために書き始めると、自然に小さくしたいと思い、より良く説明したいと思い、目を頼む前にクリーンアップしたいと思うようになります。

レビュアーの黄金律

レビュアーが変更がなぜ存在するかを理解するためにコードを読まなければならないなら、プルリクエストの説明は仕事を果たしていません。コードは何が変わったかを説明します;説明はなぜ変更が必要だったかを説明します。

小さく、焦点を絞る

プルリクエストを改善するために最もできる効果的なことは、小さくすることです。プルリクエストが大きくなるほど、徹底的にレビューされなくなり、マージに時間がかかり、より多くのバグが滑り込みます。

良いプルリクエストには一つの論理的な変更が含まれます。一つのチケットでも、一つの機能でも、一スプリントの作業でもありません——独立して理解、テスト、リバートできる一つの一貫したアイデアです。「ユーザーアバターのアップロードを追加する」は一つの変更です。「ユーザーアバターのアップロードを追加し、プロファイルコントローラーをリファクタリングし、三つの依存関係をバンプする」は一つのコートを着た三つの変更です。

プルリクエストのサイズが大きくなるにつれ、レビュー品質が下がりマージまでの時間が増える。 PR size (lines changed) EFFECT ON REVIEW Review quality (high to low) Time to merge (low to high) Small PRs < 400 lines Large PRs > 1000 lines
プルリクエストのサイズが大きくなるにつれ、レビュアーは読む代わりにざっと流し、考える代わりにゴム印を押し、バグが滑り込みます。小さなプルリクエストはより良いレビューを受けかつより速くマージされます——両方の結果が同時に改善します。

大きな作業を分割する方法

大きな機能は大きなプルリクエストとして届く必要はありません。実践的な分割戦略:

  • リファクタリングと動作変更を分ける。最初のプルリクエスト:名前変更、再構造化、抽出——新しいロジックなし。二番目のプルリクエスト:実際の機能。レビュアーはリファクタリングに副作用がないことを確認してから、新しいロジックだけに集中できます。
  • インフラを先に出荷する。データベースカラム、マイグレーション、型——すべてUIなしで追加。それからそれを使うUIを追加。各プルリクエストは独立してデプロイ可能です。
  • 機能フラグを使う。デフォルトでオフのフラグの後ろに未完成の作業をマージする。プルリクエストは小さく安全で;機能はフラグを切り替えるまで「完成」していません。
  • スタック型プルリクエスト。プルリクエストBがプルリクエストAに依存する。一部のチームは各レイヤーを独立してレビューするためにスタック型プルリクエストのツールを使います。チームがスタッキングを使わない場合は、シンプルに:まずAを完了してマージし、次にBを開きます。
役立つヒューリスティック

「and」という言葉を使わずにプルリクエストを一文で説明できない場合、おそらく複数の論理的な変更が含まれています。継ぎ目を見つけて分割しましょう。

読む価値のあるタイトルと説明を書く

タイトルは見出しです。一行で何が変わったかとなぜそれが重要かを言うべきです。「update」「fix stuff」「changes」のような曖昧な動詞を避け、具体性を目指してください。よくあるプルリクエストのタイトルの臭いとその修正方法:

プルリクエストの臭い問題より良いバージョン
「fixes」 コンテキストが一切なし——レビュアーは何が起きているか推測するためにすべての行を読まなければならない。 「同時セッションの衝突時のカートチェックアウトの競合状態を修正」
「WIP: various changes」 著者の準備ができていないことを示す。ドラフトプルリクエストはそのためにある。 本当にレビューの準備ができるまでGitHubのドラフトとして開く。
「Update UserService」 どこを、ではなく何をなぜを説明していない。すべてのプルリクエストは何かを更新する。 「UserServiceにメール変更確認フローを追加」
「JIRA-1234」 動機を理解するためだけにレビュアーが別のタブを開かなければならない。 「API レートを IP 当たり100 req/min に制限 (JIRA-1234)」——チケットはコンテキストとして、代替としてではなく。
「Refactor + new feature + bump deps」 コートを着た三つのプルリクエスト。各懸念には独自のレビューが必要。 三つの別々の焦点を絞ったプルリクエストに分割する。

説明こそが本当のコミュニケーションが起こる場所です。良い説明は、最初の10秒間にすべてのレビュアーが持つ三つの質問に答えます:

  • 何が変わり、どこで?
  • なぜこの変更が必要だったか——コンテキスト、バグ、ユーザーストーリー?
  • どうやって検証するか——テスト手順、スクリーンショット、影響するエッジケース?

具体的にするためのビフォー・アフター:

--- BEFORE (2,000行の「fixes」プルリクエスト) ---

Title: fixes

Description: (空)


--- AFTER (同じ変更、レビュアーのために書かれたもの) ---

Title: 同時セッションのカートチェックアウトの競合状態を修正

## What
同じユーザーからの二つの同時チェックアウトリクエストが、どちらも
在庫数をデクリメントする前に在庫チェックを通過してしまい、
過剰販売が生じていた。このプルリクエストは在庫デクリメントの周りに
行レベルのDBロックを追加する。

## Why
カスタマーサポートが過去30日間に12件の過剰販売インシデントを
報告した(Sentryのissue #4421参照)。根本原因はインベントリテーブルの
SELECTとUPDATEの間で二つのリクエストが競合すること。

## How to test
1. 新しい同時実行リグレッションテストを実行: npm test -- cart.concurrency
2. ステージング環境で: 二つのブラウザタブを開き、在庫=1の商品で
   「購入」を同時にクリックする。一つだけが成功するはず。

## Risk / notes
- ロックは一つの商品行にスコープされており、異なる商品が同時に
  チェックアウトする場合のスループットには影響しない。
- マイグレーション不要——既存テーブルのSELECT FOR UPDATEを使用。

コミット衛生

コミットをどのように整理するかは重要です——プロセスを満たすためだけでなく、コミットは変更がどのように生まれたかの永久的な記録だからです。一年後、誰か(おそらくあなた)がある行にgit blameを実行し、コミットハッシュを辿って歴史を調べます。彼らが見つけるものはストーリーを語るべきであり、意識の流れのように読まれるべきではありません。

良いコミットメッセージは慣例的なフォーマットに従います:短い命令形の件名行(50文字以下)、オプションの空行、そしてなぜを説明するオプションの本文——差分が既に示していることを説明するのではなく。

# 慣例的なフォーマット:
type(scope): short imperative subject

Optional body: この変更がなぜ行われたか、何の問題を解決するか、
明白でない決定とその背後にある理由。

# 実例:
fix(cart): add row-level lock to prevent oversell on concurrent checkout

feat(auth): add email-change verification flow

refactor(profile): extract avatar upload to dedicated service

メッセージのフォーマット以外に、コミットのについても考えましょう。各コミットは一つの一貫したステップを表すべきです——リファクタリング、テスト、新しい動作。個々のコミットを(差分全体ではなく)見るレビュアーは、論理的な進行を追えるべきです:「メソッドを抽出 → テストを追加 → 動作を実装」は、最後の瞬間にスクワッシュされた20個の「WIP」コミットよりも良いストーリーを語ります。

プルリクエストを開く前に、インタラクティブリベースを使って乱雑なブランチをクリーンアップしましょう。最初から完璧なコミットは必要ありません——目を頼む前に磨いてください。

レビューを依頼する前にセルフレビューをする

チームメンバーに知らせる前に、自分自身が最初のレビュアーになりましょう。GitHub(または選択したコードホスト)で差分を開き、コードを一度も見たことがないかのように読んでください。このコンテキストの切り替えは予想以上に多くの問題を発見します:残ったデバッグ出力、大きくなりすぎた関数、削除を忘れたインポート、以前は正しかったコメント。

実行する価値のあるセルフレビューチェックリスト:

  • 差分を最初から最後まで読む。ざっと流さない。初めて見るふりをする。
  • デバッグの残留物を確認する。console.log、「削除」とマークされたTODOコメント、コメントアウトされたブロック、一時的にハードコードされた値。
  • テストがあることを確認する。ロジックを追加したなら、テストはありますか?バグを修正したなら、リグレッションテストはありますか?
  • 意図しない変更を探す。空白の乱れ、再フォーマットされたファイル、偶発的なスコープクリープ。それらを別のプルリクエストに移すかリバートする。
  • 自分でガイドコメントを残す。セクションが自明でない場合、レビュアーが見る前に理由を説明するプルリクエストコメントを追加する。これは弱さの表れではありません——思いやりです。
シンプルなルール

シニアエンジニアがあなたの説明なしに今すぐプルリクエストをコールドレビューしても快適でないなら——まだ出す準備ができていません。まず修正してから、レビューを依頼しましょう。

セルフレビューはまた利己的な配当ももたらします:永久的なレビューコメントになる前に恥ずかしい間違いを発見します。「レビューリクエスト」を押した10秒後に気づいたタイポの話は誰でも持っています。10分間のセルフレビューでそのほとんどをなくせます。

レビューへの返答

プルリクエストはコードをプッシュしたときではなく、マージしたときに完了します。その二つのイベントの間にはレビューサイクルがあり、フィードバックの扱い方はコード自体と同じくらい重要です。

レビューが進み、関係が維持されるいくつかの原則:

  • すべてのコメントに返信する。「done」または「良い指摘、最新コミットで修正しました」だけでも良いです。沈黙はレビュアーに自分のコメントを見たかどうか疑問に思わせます。
  • プッシュするときに修正を説明する。「最新コミットで修正——ロジックをヘルパーに移してテストを追加しました」は、コンテキストのない新しいコミットよりはるかに明確です。
  • スレッドを思慮深くリゾルブする。ほとんどのチームでは、コメンターが満足したときにリゾルブします。フィードバックを認めずに自己リゾルブしないでください。慣例が不明な場合は聞いてください。
  • 丁寧に意見を言う。提案が間違っていると思うなら、無視せずに理由とともに明確にそれを言ってください。「そのアプローチを検討しましたが、[理由]です——まだ強く感じるなら話し合いましょう」はプロフェッショナルで生産的な会話を保ちます。レビュアーの視点からの同じ会話についてはコードを優しくレビューする方法も参照してください。
  • レビューを明示的に再依頼する。フィードバックに対処した後、レビュアーが気づくのを待たないでください。レビューを再依頼し、短い要約を残してください:「すべてのコメントに対処しました。主な変更:ユーティルを抽出し、二つのテストを追加し、XはYの理由で元のアプローチを維持しました。」

チームの規模によってプルリクエストの規範がどう変わるか

良いプルリクエストとは何かは固定ではありません——チームが成長しコードベースが成熟するにつれて変化します。期待がどのように進化するか:

チームのコンテキスト最も重要なこと実践的な規範
ソロ / フリーランス 未来の自分がレビュアーです。プルリクエストは六ヶ月後にコードに戻ったときのための意思決定のドキュメントです。 レビュアーなしでも:明確なタイトル、説明に短い「なぜ」を。後でgit logを読みやすくするコミットメッセージ。
小さなチーム(2〜8人) スピードが重要。全員がコードベースを知っている。レビューは会話的。信頼が高い。 プルリクエストを小さく速く動かし続ける。説明は軽くて良い——一〜二文のコンテキスト。同日の返答が非同期レビューの合理的な期待です。
中規模チーム(10〜50人) レビュアーが全体のコンテキストを知らないかもしれない。オンボーディングが継続的。変更がチームをまたぐ。 完全なwhat/why/howの説明。UIの作業にはスクリーンショット。テスト計画。チーム横断の変更は特別な注意と場合によってはより広いレビューが必要。
大規模 / エンタープライズ(50人以上) レビュアーは時として見知らぬ人。コンプライアンス、監査証跡、正しさのレビューが重要。ターンアラウンドのSLAが存在する。 ツールによって強制される正式なプルリクエストテンプレート。必須の承認。機密変更のセキュリティレビュー。設計ドキュメントとチケットへのリンク。リスクの高い変更のロールバック計画。

4人のスタートアップのチームはすべてのプルリクエントの説明にロールバック計画を必要としません。銀行は必要です。ステークスに合わせて形式を調整し、チームが成長するにつれて規範を見直してください——10人のエンジニアで機能したものが50人では不十分に感じられます。

まとめ

  • プルリクエストは人間へのメッセージです。自分のためではなくレビュアーのために書きましょう。彼らの時間とコンテキストは限られています。
  • 小さく焦点を絞る。プルリクエストごとに一つの論理的な変更。リファクタリングと動作変更を分ける。機能フラグを使って未完成の作業を安全にトグルの後ろにマージする。
  • タイトル + 説明 = オリエンテーション。何が変わったか、なぜ変更が必要だったか、どう検証するかに答えましょう。コードはwhatを示し;説明はwhyを提供します。
  • クリーンなコミット履歴はストーリーを語ります。慣例的なメッセージ、論理的なコミット、レビューを依頼する前のきれいなリベース。
  • まずセルフレビューをする。GitHubで自分の差分を誰かが見る前に読みましょう。ガイドコメントを残しましょう。明らかな間違いを自分で発見しましょう。
  • フィードバックのループを閉じる。すべてのコメントに返信し、コメントに対処した後でレビューを再依頼し、感情ではなく理由と共に意見を言いましょう——沈黙ではなく。
  • 形式をチームの規模に合わせる。4人のスタートアップと500人の銀行では異なるプルリクエントの規範が必要です。成長するにつれて見直しましょう。

次のステップはテーブルの反対側です:あなたのプルリクエントを受け取った人が、刺さるのではなく助けになるフィードバックをどう与えるか。丁寧に作ったプルリクエントに対して「これは間違っています、書き直してください」と返ってきたことがあれば、届くフィードバック——この記事のフォローアップ——が理解できるでしょう。

よくある質問

プルリクエストはどのくらいの大きさにすべきですか?
目安として、変更されたコードが400行以下を目指してください。さらに重要なのは、生の行数ではなく論理的なスコープで考えることです:一つのプルリクエントには、独立して理解、テスト、リバートできる一つの一貫した変更が含まれるべきです。「and」という言葉を使わずにプルリクエントを一文で説明できない場合、おそらく大きすぎます。
良いプルリクエントの説明の条件は何ですか?
良い説明は、レビュアーが最初の10秒間に持つ三つの質問に答えます:何が変わり、どこで?なぜこの変更が必要でしたか?どうやって動作を確認しますか?具体的には:変更をまとめた短い「What」セクション、動機やバグのコンテキストを示す「Why」セクション、再現手順やテストコマンドが含まれる「How to test」セクション。UI変更にはスクリーンショットが百文字に値します。機密変更にはリスクとロールバックのメモが追加価値をもたらします。
大きなプルリクエントをどう分割すればいいですか?
三つの戦略がうまく組み合わさります。まず、リファクタリングと動作変更を分ける:新しいロジックなしで一つのプルリクエントで再構造化し、次のプルリクエントで機能を追加する。次に、インフラを先に出荷する——マイグレーション、型、足場を——それを使うUIの前に出荷し、各プルリクエントが独立してデプロイ可能にする。第三に、機能フラグを使ってデフォルトでオフのトグルの後ろに未完成の作業を安全にマージする。スタック型プルリクエント(プルリクエントBがプルリクエントAをベースにしている)は、チームがツールのサポートを持っている場合のより高度なオプションです。
自分のプルリクエントを最初にレビューすべきですか?
はい、常に。レビューを依頼する前に、コードホストで差分を開き、初めて見るかのように読んでください。残ったデバッグ出力、欠けているテスト、意図しない空白の変更、自明でないロジックを探してください。レビュアーが来る前に文脈が必要なものにガイドコメントを残してください。セルフレビューは、恒久的なレビューコメントになる前に恥ずかしい間違いを発見し、レビュアーの時間への敬意を示します。
コミットメッセージに何を含めるべきですか?
コミットメッセージには、コミットが何をするかを説明する短い命令形の件名行(50文字以下)が必要です。慣例的コミットフォーマットを使ってください:type(scope): subject——例:'fix(cart): add row-level lock to prevent oversell'。差分からなぜが明らかでない場合は、件名の後に空行を追加し、動機や明白でない設計上の決定を説明する短い本文を加えてください。差分は何が変わったかを示します;メッセージはなぜを説明すべきです。