自分のレビュー手順、および心がけてること

初めに、自分がレビューの手順、次に、レビューする際に心がけてることを共有したいと思います。
GitHubとJetBrainsのIDE(PhpStorm)の利用を前提としています。

レビューの手順

レビュー前の準備

• 開発用とは別で、レビュー用で git clone でローカルにダウンロードしておく
  • 後述の「実装にわからない点があったら、自分でコメント追加したり、軽く書き直してみたりみる」の際に、別ウィンドウで、このレポジトリのdevelopブランチでエディタ開いておくと、差分確認が楽になるので

レビュー

初回レビュー

1. レビュー依頼の通知がSlackから来る
2. GitHub上で、対象のPRのdescriptionを確認
   • descriptionに、イシューチケットのリンクを貼ることになってるので、そこで仕様や要件を確認する
   • descriptionに、関連PRのリンクを貼ることになってるので、それを確認して、依存元になってそうなPRからレビューを行う
     • 例) 新機能の開発の場合、データ構造が根幹になるはずので、まずはmigrationファイルからレビューを行う
3. レビュー対象のPRをローカルにチェックアウトする
4. PhpStormのプルリクエストツールウィンドウから、変更対象ファイルを開いて確認していく
   • 下記のスクショのように、左側のサイドバーが開くので、ここからファイルを開く
     
     (https://pleiades.io/help/idea/work-with-github-pull-requests.html#provide-feedback から引用)
   • その際、変更された行の左がピンクで表示されてるので参考にする
     
     (https://pleiades.io/help/idea/work-with-github-pull-requests.html#provide-feedback から引用)
   • 変更箇所から呼ばれてるメソッド、及び変更箇所を含むメソッドやクラスの参照元にコードジャンプとかしてみる
   • コードから挙動がイメージできなければ、実際に動かしてみる
   • 実装にわからない点があったら、自分でコメント追加したり、軽く書き直してみたりみる
     • 基本、書き捨て前提
     • ただ「今の実装だと、イシューの要件満たせない」みたいな場合なら、別ブランチ切り出して、レビューで「こんな感じの実装もありかと思ったんですが、どうでしょう？」的なことはやる。
       • その時は「別に捨てて貰って良い」というスタンスを、あわせてコメントで書いておく
     • この過程で、仕様に関する疑問点等が浮かぶこともあるので、その際はイシューチケットで確認したりもする
5. PhpStormのプルリクエストツールウィンドウから、変更差分を別Windowで開いて、もう一度確認していく
   • （わかりにくいけど）下記のスクショように、GitHubのプルリクエストのFiles changedと似たような画面が開く
     
     (https://pleiades.io/help/idea/work-with-github-pull-requests.html#provide-feedback から引用)
   • おおよそは「4. PhpStormのプルリクエストツールウィンドウから、変更対象ファイルを開いて確認していく」で見れているはずなので、ここでは「変更前のコードと見比べて、なにか新しい気づきはあるかな？」くらいの温度感で、さらっと見るイメージ

２回目以降レビュー

（タノムでは、初回のレビューでコメントを貰って、その修正が終わったら、再度レビュー依頼を投げる運用になっています）

1. レビュー依頼の通知がSlackから来る
2. レビュー対象のPRをローカルにチェックアウトする
3. GitHub上で、Files changedから、前回レビューとの差分コミットを抽出できるので、それを見ながらレビューを行う
   • Files changedの下の「Changes from all commits」から「show changes since last review」から、前回とのレビュー差分だけを抽出できるので、だいたいそれを使う
     • ただ、rebaseやmergeが行われると、前回レビューとの差分以外の変更が表示されることがある
       • その場合、ローカルでレビュー対象のブランチから別ブランチを切って、前回レビュー時点のコミットまで git reset して、まとめて1コミットにして、そのコミットを指定してdevelopブランチに git rebase --onto 的なことを、稀にする。
         • ただ、これはrebaseミスが発生しうるので、これをやる際は、怪しいファイルは初回レビューでの４番の方式で見ていく。そして結果、全てがめんどくさくなって、差分レビューを諦めて、すべて初回レビュー４番の方式で見ていく、ということもままある。
   • 初回レビューと同じで、コードから挙動がイメージできなければ、ローカルで動作を見たり書いたりする

レビューする際に心がけてること

• 「思ったこと」と「レビューコメントに書くこと」は分けて考える
  • （「しきい値が難しいという」結論に落ち着く話だと思いつつも）基本的には「違和感あるなー」位のレベルだったら指摘しない
    • 手元で書いたり動かすのは、あくまで、コードの理解を第一として、あら捜しのためでない
      • 自分で書いた結果、コードが理解できたらそれで良し。その場合、自分で書いたコードは捨てる意識。
        • ただNITS的に「今のアプリの他の実装でよく見るのはこんな感じです。修正要望でなくて参考情報として。」みたいなことは書くことはある。これも捨てられる前提。
      • とはいえ、不明点とか不具合があった場合、それを具体例に使って「XXXという場合、YYYとなるのが気になりました。その場合、ちゃんと試してないんですが、下記コードならそうはならない気がして、ちょっと見てほしいです（貼ったコードは捨てていいです）」みたいなコメントは書く
• NITS的なレビューコメントを書く際には「（どうでもいい寄りの話ですが）」「（どっちでもいい話ですが）」的な、自分の温度感をレビューコメントの先頭に書く
  • 以前に [NITS] みたいな、よくあるやつを書いてみたこともあったけど、「その英単語が何を示してるか」が覚えられなかったので、括弧書きで、素直に自分の温度感を書くようにした
• レビュー中の動作確認は「コード読んでて気になったところをローカルにあるデータの１パターンだけ動かしてみる」みたいな軽いやつ。「一通りを動かしてみる」まではいかないレベル。
• 「ここコメントあったら嬉しいかも」を思ったら、積極的に伝える
  • 自分のスタンスとして、内容が明らかに間違ってる、とかでないなら、コメントの質とか気にせず、いくらあっても良いと思ってるので
• 実装される機能の内容や、実装者によって、レビューの濃淡を変えるけど、勘所は押さえるようにする
  • 例えば「エラーメッセージの文言修正」はGitHub上でレビューを終わらせるし、「実装者がチームにジョインして最初のプルリクエスト」ならしっかりレビューする、みたいな濃淡をつける。
    • 「ある程度抜いてレビューする」際は、「ここで万が一があったらサービス止まる」「migrationで間違いがあったら後追いの修正が難しい」みたいな勘所を意識してレビューするようしてる。

今後のレビューでより良くしたいこと

• ポジティブフィードバック

これを書くにあたって、レビュープロセスに関して簡単に調べたのですが、その中で、自分はポジティブフィードバックが弱いと思ったで、意識的に動きたいと思います。