レビューを出す場合

関心事（目的）を少なく（1つに）する

機能の変更でフォーマットの変更はしない
テストと実装は片方だけ（新規実装は除く）

[!tips] 大きなPRを切り出す
大規模な実装をするとき（リファクタリングをするときなど）は、最初から1つの関心事に絞るのは難しい。
そういう場合は関心事がごった煮になった大きいPRをDraftで作って、その後適切な関心ごとの単位を見つけて切り出していけば良い
git checkout <branch> <file> などを使うと別 branch の状態の file を checkout (気持ちの上では copy) できて一部楽になります。

Pull Request を chain する

あるやりたいことのためにn個の直列な PR を作りたくなったら、k + 1番目の PR の base branch を k番目の branch にするようにしています。そうすると diff が小さく見えるのに加えて、k番目が merge されると自動的に base branch が default branch に切り替わるので何もしなくても勝手にいい感じになります。関心事を小さくすると Review 速度のほうが PR 作成速度より早くなるので、新しい PR を積むときに一番古いものの Draft を外していくだけで良いペースで消化されていきます。

自分でセルフレビューする

• 想定外の動作変更が無いか
  • 動作変わらないつもりで書き換えていても変わることはよくある
    • 変更したかったことだけじゃなくて、実際に変更したことから逆説的にも行う
  • なんだこの実装？みたいに考えて表面的な動作だけ見て安直に修正することもあるが、大抵はそうなっている理由がある
  • もちろんより良い実装に直せる場合もあるが、そうなった意図を考えて手を加えること
• src => distのマージの前にローカルでdist => srcのマージと動作確認を行う
  • conflict発生してなくても、影響がある場合がある
  • 競合として認識されない = 上書きされるのでむしろ危険
• ビルドできるかを確認する
• 全てのテストがパスするかを確認する

書き加える情報

理由

検証情報

ファイル間の関連

PRの立ち位置

レビューをうけるとき

• 「不具合・仕様が間違っている」以外のレビューコメントをそのまま実装するのはあまりよろしくない
  • 「こんな感じはどう？」「ちょっと気になる」という軽い気持ちのコメント（or レビュワーが見当違いなことを言ってるだけ）なので、本当にそれが正しいかは実装して色々試してみないとわからない（特に関数・変数名はその場の思いつきで書いてる）
• 今までの開発チームの中で一番生産性があがったルール「PRのレビューは割り込みであってもすべてのタスクより優先でやる」ってやつだった。最初は自分のタスク中断されて「うっ……」てなってたけど、他人のタスクを止めないし、結果的に自分の時も待ち時間が発生せず、チーム全体の空気もよくなった
• コードレビューのチェックリスト

綺麗な履歴を残す

参考

• より良いプルリクエストのための10のヒント
• Pull Request の関心事は一つにしよう]
• LGTM (Looks Good To Me - 良さそうだね) がなぜレビュー承認に使われるのか
• コードレビューはつまらないから丁寧なプルリクエストでチームの生産性向上を目指す
• Googleのソフトウェアエンジニアリングから学ぶコードレビュー
• 「コードレビューたまりがち問題」を解決するには
• プルリクエストにコメントすると停滞してしまうのを見ると、「プルリクエスト単体で完成度あげるよりマージして全体の中で整合とった方がいい」派の私は、プルリクエストのコメントを控えるようになってしまう。