プルリクエストレビューチェックリスト

how-to-review-purll-requests.md

• 全体的な意図。なにをどうやって実現するのか(What, How)。
• 変更の文脈、位置付け。なぜこの変更が必要なのか(Why)。
• 複数の独立した問題が含まれているか
  • 含まれている場合、分割できないか検討する
• 設計の誤りは影響が後を引く可能性があるので、なるべくちゃんと見ておきたい。とくに永続化されるデータ構造のミスは、リリースしてしまうと修正な面倒なので、注意する必要がある。例えば:
  • SQLアンチパターンに該当するようなテーブル設計になっている(実データが発生するとめんどう)
  • 本来別のAPIを新設すべきところを、既存APIへの追加パラメータで無理矢理処理している(修正される挙動への依存が増えるとめんどう)
  • 手続の種類が増えても修正不要なように書ける(一度書けば済む)のに、手続の数だけコード追加が必要な設計になっている(無駄な手数が増える)
• 追加・変更される挙動について、テストケースが追加・変更されているか、テストケースの見出しレベルで簡単に見る
• 特殊な理由のある変更など、コードだけから理解できなさそうな変更は、コメントに経緯が書かれているかを見る
• UIの変更が含まれる場合、スクリーンショットが添付されているか
• QA手順が記載されているか
• これ以外のすべて: セキュリティー、局所的なコード品質などは最悪見なくてもいい
  • 効率よくレビューしつつ、セキュリティーをいかに担保するのかという点については、いまのところどうすればいいのかわからない。時間をかけずに効率よくレビューするという命題と、セキュリティー検査をしっかり行うという命題は相容れない気がする。