原文 10 tips for better Pull Requests by Mark Seemann
良いプルリクエストを作ることには、良いコードを書くこと以上を含んでいる。
プルリクエストモデルは、チームでソフトウェアを開発するための素晴らしい方法になりつつある。チームメンバーが分散している場合は特にそうで、オープンソースの開発だけでなく、企業においてもそれは同じことだ。2010年頃から私は、オープンソースプロジェクトにおいてだけでなく、クローズドソースのソフトウェア開発のために内部的にプルリクエストのワークフローを使っている顧客のチームメンバーとしても、プルリクエストをレビューしてきた。
その中で、たくさんの素晴らしいプルリクエストも見てきたし、一方で改善が必要なプルリクエストも見てきた。
良いプルリクエストには、単なるコード以上のものが含まれている。多くの場合、そのプルリクエストがコードベースに取り込むにふさわしいかどうかを判断するために、1人あるいは複数人のレビュアーが関わる事になる。つまり、あなたは良いコードを書くことはもちろん、レビューを行う人達のお眼鏡にもかなう必要があるのだ。
ここに、あなたのプルリクエストをより良くするためのヒントを挙げよう。全部網羅しているわけではないが、良いプルリクエストを作るために重要ないくつかの視点を提供できるのではと思う。
1. 小さくまとめる
小さく、焦点を絞ったプルリクエストは、承認される可能性が非常に高い。
私がプルリクエストの通知を受け取って最初にするのは、そのサイズ感を知るためにざっと目を通すことだ。プルリクエストを正しくレビューするには時間がかかるが、経験上、プルリクエストの大きさに対してレビュー時間は指数関数的に長くなってしまう。この2つの関係は線形ではないのだ。
私がオープンソースプロジェクトで巨大なプルリクエストを受け取った時は、申請者は既にかなりの作業量をそこに割いた可能性が高いと考えて、例えプルリクエストが大きすぎてもそれをレビューする時間を惜しむ事はない。申請者にとってそれが初めての貢献であると思われる場合は特にそうしている。しかしそれでもプルリクエストが大きすぎる場合、レビューのための時間をスケジュールする必要がある。大きな塊のコードを、ある部分を5分でレビューし、別の部分を5分でレビューし…といった事はできないので、連続した時間を確保する必要があるからだ。このために、レビューのプロセスに既に遅れが出ている事が分かるだろう。
これが業務でなら(例えば申請者がコードを書いてお金をもらっているなら)、大きさが適切でないという理由でそのプルリクエストをリジェクトしてしまうのもよくあることだ。
ではどのくらいの大きさなら十分に小さいと言えるだろうか？もちろん、それはそのプルリクエストが何についてのものなのかによるが、1ダース(12)以下のファイルしか変更していないようなら、まあ悪くないと言うべきだろう。
2. 1つの事だけやる
単一責任の原則で、1つのクラスは1つの責任(役割)のみを持つとされているように、1つのプルリクエストは1つのテーマのみを扱うべきだ。
逆に、あなたは3つの独立した別々のテーマ(テーマA、B、Cとする)を扱う1つのプルリクエストを投げた場合を考えてみよう。レビュアーは、AとCについては適切でやり方も正しいとすぐに賛同してくれた。しかし、Bについては問題があると言う。レビュアーは、Bはそもそも取り扱うべき問題ではないと言うかもしれないし、あるいはやり方に賛成できないと言うかもしれない。
こうなると、B自体をどうするか、どういうやり方がいいのかという長い議論が始まってしまう。こういった議論は、合意に達するまで何日もかかる事もあり得る(議論の参加者が違うタイムゾーンにいる場合は特にその傾向がある)。レビュアーの意図を盛り込むために、プルリクエストに修正を加える必要があるかもしれない。どれも時間がかかってしまう。
その議論が行われている一方で、他のコミットをmasterにマージするのにも時間が必要なので、あなたのプルリクエストは自動的にマージできなくなるまで遅れていってしまう。ようこそ、マージ地獄へ。
こういった事が起きている間、完璧に仕様を満たしているAとC向けのコードは、コードベースに何の価値ももたらさないまま、あなたのプルリクエストの中に留まったままだ。
その代わり、AとBとCのそれぞれに対応した独立した3つのプルリクエストを投げた場合、AとCに賛同したレビュアーは、その2つのプルリクエストをすぐに取り込んでくれるだろう。この方法だと、特に議論が必要のない部分のコードは、すぐにコードベースに価値をもたらすようになるわけだ。
1つのプルリクエストで多くのテーマを扱えば扱う程、その部分のどこかの取り込みが待たされる可能性が高くなる。プルリクエストごとに1つのことだけをするようにしよう。これは、プルリクエストを小さく保つのにも意味がある。
3. 行の幅に注意する
プルリクエストのレビュアーは、多くの場合diffツールでレビューを行うだろう。GitHubやStashは、どちらもレビューの際にブラウザベースでdiff形式の表示をしてくれる。レビュアーは左右に並べてdiffを表示するようにもでき、どのような変更が含まれているかを理解するのにはこれが分かりやすい。しかしこれだと、画面の半分だけでコードが読めるようになっている必要がある。
つまり、行の幅が広いと、レビュアーに横スクロールを強いることになってしまうのだ。
それ以外にも行の幅を80文字に押さえるべき理由はたくさんある。この理由の一覧に、コードをレビューしやすくするというのも加わったと考えよう。
4. フォーマットの変更はしない
既にあるコードを「自分の」スタイルに合わせるようにフォーマットを変えてしまいたくなるかもしれない。しかしそれは思いとどまって欲しい。
diffの結果には、ソースコード内で変更した全バイトが表示される。いくつかのdiffビュアーには空白の変更を無視するオプションがついているものもあるが、それが有効だとしても限界がある。特に、コードを移動してしまったらおしまいだ。だから、そういうことはしないで欲しい。
もし空白に関する問題を指摘する必要があるなら、ファイル内で位置を変更したりフォーマットを変更したりコードのスタイルを変更して、そういった問題だけを扱うプルリクエストを投げるようにしてほしい。そして、プルリクエスト内でそれについてコメントしておくのだ。
5. ビルドできるコードをのみを含める
プルリクエストを申請する前に、自分のマシン上でビルドしておこう。「自分のマシンでは動いたんだ」というのは正直役に立たないのだが、これは最低限のことだ。「自分のマシンでも動かない」なら、他のマシンで動くことはまずないだろう。
コンパイラの警告メッセージにも注意しよう。コンパイルできないわけではないので、注意して見ないと気づかないこともあるだろう。しかし、もしあなたのプルリクエストが(たくさんの)コンパイル警告を出すようなら。レビュアーはリジェクトするかもしれない。少なくとも私はそうしている。
関わっているプロジェクトでビルドスクリプトが提供されているようなら、それを実行してみて、ビルドが成功した時だけプルリクエストを申請すること。私のオープンソースプロジェクトの多くでは、(コンパイル以外も含め)警告もエラーとして扱うようにしたビルドスクリプトを用意している。そういったビルドスクリプトがあると、コードベースに対して色々なルールを自動化して適用できる。あなたのブランチをマージする際にはレビュアーもそのスクリプトを使うだろうから、あなたもプルリクエストを申請する前にそうしておこう。
6. 全てのテストをパスさせる
コードベースが自動テストに対応しているなら、プルリクエストを出す前に全てのテストにパスしている事を確認しよう。
こんなことは言うまでもないかもしれないが、1か所どころか複数箇所でテストに失敗するプルリクエストを受け取るのもよくあることなのだ。
7. テストを追加する
これも(ユニット)テストが自動化されていればの話だが、書いたコードに対してはテストを追加しよう。
テストのないプルリクエストを受け取るというのはそうないことだが、その場合は私はリジェクトしている。
これは絶対的なルールではない。テストカバレッジなしでコードを追加する必要がある場合もある(例えばHumble Objectを追加する時など)。しかしテスト可能なら、テストをするべきだ。
また、対象となるコードベースで既に培われてきたテスト戦略に従う必要もあるだろう。
8. 理由を書く
自己文書化したコードというのはそうあるものではない。
コードのコメントとは謝罪であるとも言うが、コメントよりも、うまく名前付けされた処理、型、値の方が、間違いなく私はいいと思う。一方で、コードを書くときには、それが自明であるかどうかの判断を下す必要もある(特にビジネス「ロジック」を扱う時)。
また、「何を」書いたのかではなく、「なぜ」その方法でコードを書いたのかを説明しよう。
コードの理由の書き方を、私が重要だと思う順に並べるとこうなる。
- 自己文書化したコード : コードの自己文書化については、あなた自身で判断を下すことが可能だ。Clean Code アジャイルソフトウェア達人の技は、まさにそれをどのようにやればいいのかを書いた本だ。
- コードのコメント : コードをうまく自己文書化できなかった場合には、コメントを付けよう。少なくとも、コメントはコードと同じ場所に書いておき、バージョン管理システムを変えるような時でもコメントが失われないようにしよう。別の方法で問題を解決するより、コメントを付けてしまう方が適している場合の例がこれだ。
- コミットメッセージ : ほとんどのバージョン管理システムでは、コミットメッセージを書くことができる。ここには最低限のことしか書かない人が多いが、コミットメッセージにもコードの根拠を書けるのだ。場合によっては、なぜその順番で作業を行ったのかを説明する必要があるかもしれない。そういったことをコードのコメントに書くのはふさわしくない一方で、コミットメッセージはちょうどいい機能だ。同じバージョン管理システムを使っている間はコミットメッセージを保持しておけるが、バージョン管理システムを乗り換える場合にはメッセージは消えてしまう点には注意しよう。詳細なコミットメッセージを書く必要があると思った例がこれだ。ただし、いつでもそうしているというわけではない。
- プルリクエストのコメント : 滅多にないだろうが、上の3つのどの方法も適していないという場面に遭遇するかもしれない。GitHubやStashなどのプルリクエスト管理システムでは、プルリクエスト自身にもメッセージをつけることができる。このメッセージは実際のソースコードから2段階も離れてしまっているし、同じホストを使っている間しか保持されない。例えばCodePlexからGitHubに移行すると、プルリクエストのコメントは失われてしまうのだ。それでも時には自分のことをレビュアーに説明する必要がある場面もある。ただし、ソースコード外の何かについて説明することになるだろう。この方法が適していると思われる例がこれだ。
自明なことを説明する必要はないが、慎重には慎重を期した方がいい。今日あなたにとって自明なことは、他人には自明ではないかもしれないし、あるいは3ヶ月後のあなた自身にとってもどうか分からないのだから。
9. 上手に書く
良いコードを書こう。そして上手な作文をしよう。これは主観的な面もあるが、コードにも作文にもルールがある。コードには正しさのルールがある。もしそれに従わなければ、コンパイルできない(インタプリタ言語なら実行に失敗する)。
また一方で、コードのコメントやコミットメッセージ、プルリクエストのメッセージといった作文にも同じことが当てはまる。
正しいスペル、正しい文法を使い、正しく句読点を使おう。これに従わないと、あなたの文章は理解してもらいにくくなる。レビュアーだって人間だ。
10. スラッシングを避ける
(訳注 : 筆者は、コードベースに直接の意味をもたらさないコミットでコミット履歴が汚れることをスラッシングと呼んでいるようだ)
場合によっては、レビュアーはあなたのプルリクエストの様々な問題を指摘してくるだろう。そしてあなたはそれに従うことになる。
これによって、プルリクエストのブランチにコミットを追加する必要が出てくる。これ自体には何ら問題はないのだが、不当な混乱の元になる可能性がある。
例えば、プルリクエストにAからEまでの5つのコミットが含まれているとしよう。レビュアーはコミットBとCの中身が気に食わなかったので、そのコードの削除を要求してきた。大抵の人は、プルリクエストのブランチをチェックアウトして、指摘されたコードを削除して、別のコミット(F)としてコミットリスト[A, B, C, D, E, F]となるように追加するだろう。
しかし、なぜ必要のないコードを追加し、それからまた削除するというコミットをマージするのだろうか？これがスラッシングだ。つまり、何の価値も提供しないコミットだ。
その代わりに、指摘されたコミットを削除して、[A, D, E]のみを含むよう変更したブランチを強制プッシュしよう。レビュー中、あなたはそのブランチの完全なオーナーなのだから、自由に変更して強制プッシュしてよいのだ。
もう1つ私がよく見るスラッシングの例は、プルリクエストが古くなってきた時に起きる(長い議論のためであることが多い)。こういった場合、プルリクエストが最新に追いつくように、オーナーがmasterブランチと定期的にマージしたりする。
もう一度言おう。マージするコミットも全部必要なのだろうか？あなたはそのブランチのオーナーなのだから、プルリクエストのブランチをリベースして、強制プッシュして欲しい。そうすれば、最終的なコミット結果はきれいになる。
まとめ
あなたのプルリクエストをレビューするのに、1人あるいは複数人のレビュアーが関わることになる。レビュアーに余計な作業をさせないようにしよう。
レビュアーに余計な作業をさせればさせる程、あなたのプルリクエストはリジェクトされる可能性が高くなるのだ。
