レビュアーにやさしいリファクタリングPRを作る
リファクタリングの PR、見るのツラい内容になりがち
PR(PullReqeust)を作成してレビューを受け、Approve を受けたらマージする..という開発スタイルはよくあるパターンで、新たな機能追加や修正では観点が明確で動作確認も実施しやすいのですが、これがリファクタリングがテーマになると、途端にレビューが大変になることがあります。
個人的な経験則もありますが、何も意識せずに PR を作ると、次のような問題が発生しやすいように感じます。
- １テーマに関する修正が一気に詰め込まれていて物量が多い
- 何を確認したらよいのかわからない
- 複数 PR に分けている場合に、後続の PR だけを見ても理解できない
- など...
リファクタリングの PR は内容も淡々としたものになることが多く、確認もリグレッションテストが中心で、レビュアーはそこそこ心を削られます。そのうえ上記のような問題を抱えていると、全然レビューしてもらえない・レビューしてもらえてもざっくりした確認になってしまう、などといった状態に陥ります。
そういった問題を緩和するため、PR 作成時に工夫している点を整理してみました。何気にリファクタリングに限らない話も多分に含んでいますが、そこは大目に見てください。
なお、これを書いてる私自身も、つい巨大 PR を作り、Description に「すいません！！」と書くこともあるため、自戒の意味もあります。
コミット・PR を小さくする
レビューしやすい PR を作るための基本としてよく挙げられるのが、とにかく小さくすることです。自分の場合「この PR は ◯◯ を変更します」と一文で書いてすぐ理解できる程度を理想にしています。
小さいほど影響範囲が狭くなり、チェックする観点も少なく済みます。また、レビューにかかる時間も短く済むため、他の修正とコンフリクトするリスクも減ります。
レビュアーがレビューに費やせる時間も無限に存在するわけではないため、巨大な PR では着手するまでに一定の余裕を要求することになります。（一目見たときに「ｳｯ」となって後回しにされがちだったりもする）
小さい PR であれば空いた時間にサクっと見ることができるため、回転が速くなりやすいです。
分割は意味のある単位でやる
小さくすることは効果的ですが、やみくもに小さくすれば良いというわけではなく、意味のある単位で分割することも大事です。極端な例ですが、何らかの処理とそれに対応するテストコードを追加するとして、それらは同一 PR に含まれていた方が確認しやすいでしょう。
また、関数の追加時などは、呼び出す箇所の修正も一部含めておいた方が、どう利用されるのかのイメージがつきやすかったりします。
感覚としては、「PR の小ささを保てる範囲内で、PR の外で確認しないといけない情報を減らす」というのを意識しています。
機械的な変更と手動での変更はコミット orPR を分ける
リファクタリングにおいて、機械的なツールを用いたコード修正を行うことがあります。たとえば次のようなものです。
- ESLint を用いた
eslint --fixでの autofix - Prettier での整形
- ライブラリ更新時のマイグレーションツールの実行
ただ、ツールでの機械的な変更は一部で問題が発生することも珍しくなく、別途手動での追加変更が必要になることも多々あります。「Lint を適用したものの、事情があり一部無効化したい」 「マイグレーションツールでは対応しきれず手動で更新しないといけない」など、理由はさまざまです。
そういった際に、機械的な変更と手動での変更はコミット orPR を別にするとレビュアーに優しい内容になります。
機械的な変更と手動変更を分けることで、PR 作成者が意図して変更を加えた箇所がどこなのかが一目でわかり、特に注意してチェックすべき点が見えてきます。追々何か問題が発生したときにも、ツールの問題なのか、人が加えた変更の問題だったのかの切り分けが容易となります。
移動・リネームと内容の変更はコミット orPR を分ける
ファイルの移動・リネームによる変更と、ファイル自体の内容の変更はコミット orPR を分けた方がよいです。移動・リネームでは、多くのケースでコード間の依存関係の更新も同時に発生します。JS を例に挙げると、移動・リネームを行うと、利用箇所の import xxx from 'xxx' はすべて変更が必要となります。
しかし、これらの変更は規則的であり、変更量が多くてもそれほどレビューは難しくなかったりします。
ただ、ここに別の内容の変更も混ぜてしまうと、規則的だったはずの変更の一行一行が「何か意図して別途追加されている変更か？」という点を考慮して見る必要が生じ、量の多い規則的な変更に埋もれて確認が漏れてしまうリスクも増します。
移動・リネームだけで 1 コミット orPR にまとめておき、1 回でチェックすべき観点をひとつに絞り込むことで、レビュアーの負荷を軽減することができます。
なお、 git mv を利用せずにファイルの移動・リネームをした場合にはさらにヒドイことになります。完全に新規ファイルとして認識され、ファイル内容すべてが Diff 扱いになり、もはや何が何だかわからなくなります。移動・リネームによって履歴が途切れていないかも注意が必要です。
途中で見つけた他の改善点をすぐに全部拾おうとしない
あるテーマについてリファクタリングを進めていると、他にも気になる箇所が出てきて、全部直したい衝動に駆られることがあります。
ボーイスカウト・ルール[1] という言葉もあるため、見つけた改善点に対処すること自体は歓迎されるべきことかと思いますが、単一 PR で考えた場合には、ほどほどに留めるのが無難です。
変数の typo や簡単なコードフォーマットの修正などの軽微なものであればそれほど影響はないかと思いますが、「ライブラリのバージョンアップの対応だけど、コード重複の共通化もやりました！！」みたいなことは避けたほうがよいです。
自分の場合は FIXME コメントを残すに留めておき、後々別の PR として対処したりしています。
複数テーマのリファクタリングを並行して進めない
PR の作り方の話からは少し逸れますが、コードベース全体に関わるようなリファクタリングに着手する場合、それがひと段落つくまでは他テーマには着手しないほうがよいです。
一定規模の対応時には、少なからずコンフリクトが発生します。複数テーマを並行して対処していると、コンフリクト解消だけでも一苦労、といったことも招きかねませんし、レビューする側も思考のスイッチが大変です。
「少なくとも ◯◯ まで対処したら一旦完了とする」といったチェックポイントを設けておくとよいかもしれません。
確認した点・不安な点・特に見てほしい点を明記する
PR の Description に自分自身でチェックした項目・不安なので特に見て欲しいことを明記しておくのも効果的です。
すでにチェック済みな点が書いてあることで、優先度が低い箇所を何人も重複して見てしまうことを防げますし、レビュアーのコストを特に重要度が高い点に集中させることができます。
また、特に見て欲しい点が書いてあることで、すべては確認できずとも、「ここだけ動作確認してコメント残しておこうかな」という部分的なレビューをするための取っ掛かりにもなり、受け口が広くなります。
レビュアーの理解度にあわせて確認してほしい点を書いておく
まれに何でもわかる超人はいますが、そういった人でない限りはレビュアーにも得意・不得意があるため、ものによっては全体を把握しての指摘が難しいケースもあります。
そういった場合でも、簡単に確認できる観点があればそれを明記しておくと、特定個人しか確認できないといった状態を緩和できます。また、簡単な狭い範囲でも見てもらうことで徐々に理解に繋がるため、長期的に見て知見を共有する意味でも効果的です。自分だと「〜画面が影響を受けるので、最低限動作確認だけでも嬉しいです」といった具合のコメントを書いておくことが多いです。
※なお、そもそもが難解なテーマなときは、あらかじめ時間をとって前提知識を共有しておくなどの手段を取ることもあります。
前後 PR と重複内容があっても Description を省略しない
リファクタリングにおいてコードベース全体を修正する場合には、PR を大量に作成することもあります。その場合、1 件の PR だけでレビュー可能なことを意識するとよいです。
何件も PR を作っていると、面倒なので後続の PR では Description に「#xxx と同様です」のように省略して書きたくなります。ただ、チーム規模にもよりますが、すべてのレビュアーがすべての PR を追えているケースはなかなか珍しいです。リファクタリングの流れの中で作られた PR だとしても、レビュアーにとっては常に「このテーマで最初にレビューする PR」になる可能性がある点を考慮したほうがよいです。
Description が省略されていると、今見ようとしている PR の外側に前提知識を得るために探しに行くコストが増大します。また、時間が経ってから PR を見返すこともあり、その際にも単一の PR を見るだけで全体像を把握できると色々と捗ります。
テーマがそもそも何なのか・前提条件となる資料・全体の中でどこにあたるのか、は全ての PR に記載しておいてもよいでしょう。
色々書きましたが、正直頭で理解していても、私自身もつい面倒になって１ PR に色々と突っ込んじゃうときもあって、完璧にはやれてないな〜と日々反省しています。
己を律してレビューしやすい PR を作っていきたい所存です。
もし他にも「〜をやるとレビューしやすいよ！」「〜は避けた方がいいよ！」みたいなものがあればコメントもらえると嬉しいです。
Discussion