おれおれレビューしやすいプルリクエスト

Last updated at 2019-12-20Posted at 2019-12-20

はじめに

オープンロジのエンジニアで @ttaka と申します。
この記事は OPENLOGI Advent Calendar 2019 の20日目になります。

弊社では古株にあたるため、プルリクエストのコードレビューをする機会も多く、早くレビューをこなすためにこうなってたらいいなと思ってることを、ちょっとまとめてみました。

なお、あくまで個人の考えであり、OSSとして公開されているパブリックリポジトリなどのマナーとは一部異なるものもありますので、そのあたりはご留意ください

いやもうほんとこれさえやってもらえれば、あとは瑣末な問題といってもいいくらいに、これはお願いしたい！

Files changedの数がスゴいレビュー依頼がきたりすると、時間があるときにみようと思って、一旦そっ閉じしてしまいます……いや、見ますよ見ます、あとで必ず見ますけども！

レビューに時間がかかると、その分リリースも遅れ、リベースやマージ時のコンフリクト発生確率も増えていきますから、細かくリリースできる単位でプルリクエストを分割していただけると、お互い幸せになれるかなと。

ただ、大きな機能の開発のときは、比例して大きくなりがちなのもわかります。そんなときでも機能を小さく分割し、その単位でさらにブランチを切ることで、同じように小さなプルリクにできますね。

例えば、あるチケットを解決するための修正(a)を行い、その際たまたま目についた関係のない箇所のリファクタリング(b)もやったとします。

ボーイスカウト精神ほんと素晴らしい！

ただ、その修正が一緒になったコードをレビューすると、aの解決にbも何か関係があるのだろうかと悩むことになります。

リファクタリングは負債を返済していく上でも大歓迎ですので、bはぜひ別のプルリクにしてレビュー依頼していただけると！

社内のリポジトリなので、そんなに綺麗にする必要はありませんが、コミット単位で実装者の思考を追いかけながらレビューすることも多いので、上記と同じく異なる内容のコミットは別になってると嬉しいです。

また、プルリクを分けないまでも、コミットがそれぞれ意味のある単位で別れていると、その単位でレビューできるので、小さなプルリクと同じ状況を作り出せます。

ちなみに、コードフォーマッターによる修正なども、それのみのコミットに分割していただくほうが好きですね。

例えば、最大表示件数の定数定義があり、その数値を変更した際、

5から3に変更

といったコミットメッセージがあっても、変更の差分を見れば一目瞭然です。

表示が多すぎると1画面内に納まらず、ユーザビリティが悪化するため、3件までに制限

こんな感じに書いてあれば、レビューしてるときもなるほど確かにと思いますし、このことを知らない人からこの値を大きくしてほしいといった依頼があり、別の開発者が修正することになっても、本当に変更していいのか事前に気をつけることができます。

こちらもただ不具合修正といった感じに書くのではなく、何をどうしたのか的なタイトルにしていただけると、概要を把握しやすくて嬉しいです。

指摘のあった箇所の修正を既存のコミットに混ぜてforce pushされると、レビュー済みとそうでないものが混ざってしまい、どこを見ればいいのかわからなくなるので、上書きせずに追加してほしいです。

OSSのリポジトリなどでは、履歴が汚れるのを嫌って上書きを推奨してる場合もありますが……。

なお、コミット自体が不要になった時であれば、打ち消しのためのリバートコミットを作るのではなく、そのコミットを削除してforce pushしていただいて構いません。

弊社ではプルリクのテンプレート機能を使って、作成時に書くべきことのテンプレートが挿入されるようになっています。

基本的には記載してほしいものですが、単なるコードフォーマットの修正など、ほとんどレビュー不要なものであれば、その旨記載してばっさりカットして構いません。
変にテンプレートのまま残してあると、どこかきちんと記載した箇所があるのか探すことになり、無駄な時間がとられてしまいますので。

なお、書く必要のある内容なのに面倒でカットするのはダメ絶対！

上記のように作ったプルリクのレビュー依頼をだしてるのに、なかなかレビューしてもらえない場合は、忙しさにかまけて棚上げしてる可能性がありますので、遠慮なく尻を引っ叩きにきてください……すみませんすみません