読者です 読者をやめる 読者になる 読者になる

気の向くままに書き綴る

勉強会参加したメモや日々の思ったことのメモ等

良いPull Requestのための10のTips読んだ時のメモ

この記事を先週読んでセコセコ、メモしていたら、日本語の翻訳が出てた。
せっかくの機会なのでGistに貼っていたのもをブログに上げる。


原文 10 tips for better Pull Requests

Yakst - より良いプルリクエストのための10のヒント

良いPull Requestのための10のTips

良いPull Requestを作ることは良いコードを書くこと以上を伴うもの

Pull Requestモデルは、チームで開発する素晴らしい方法であることが判明している(特に分散しているチーム)
もちろんこのモデルはOSS開発だけじゃなくても、エンタープライズでも言える。
2010年位から私は、自身のOSS、顧客のいくつか、また内部のクローズド開発でもPull Requestモデルを行ってきた。

当時、私は多くの偉大なPull Requestを見てきた。
そして、いくつか改善が必要なものがあった。

良いプルリクエストは最適な良いコード以上に含まれている。
ほとんどの場合、一人または多くのレビュアーがいて、
あなたのPull requestがコードベースにフィットするかどうかレビューするんだ。

あなたは良いコードを作るだけじゃなく、レビューアーにあうか考える必要がある。

ここに、あなたのPull requestがより良くなるようにTipsを書いておく。
網羅的ではないけど、良いプルリクエストを作る上で重要な側面だと思ってる。

1. 小さく作る

 小さくすることに集中したPull Requestは取り込まれやすい
 
 大きくなってしまったら、レビューアーと調整して行ったほうが良い。
 レビューは5分毎に刻んで行うことが出来ず、まとまった時間が必要だからだ。
 Pull requestを送るものによるが、
 大体12ファイル以下の変更なら、悪く無いというべきだろう。

2. 一つのことだけをする

単一責任原則のように一つのクラスに一つの責任をもつようにPull requestもそのように一つに絞って出す必要がある。
例えば、あなたが3つの独立した機能をもつPull requestを送った場合に、
AとCは有用であり取り込むべきとなったが、Bには問題があるといったことがしばし起こる。
その場合、Bについて長い議論をし、最終的にPull requestを変更する必要な場合がある。

長い議論の間にもMasterではいくつかCommitされているおり、
既にAとCについては自動でマージできない状況になる。

AとCは既に完璧なものであったにも関わらず結局、宙に浮いている状態である。

そのかわりに、A,B,Cすべてを独立してPull Requestを送った場合、AとCはすぐに取り込まれるだろう。
一つのPullRequestは、一つのことに集中にして行うようにするべきである。

3.  一行の長さを考える

あなたのRullRequestをReviewをする際にレビューアーはDiff toolを使う。
GitHubもStashもWebベースのDiffツールを提供している。
レビューアーは提供されている画面分割されたDiffをみている
それはCodeを画面半分でも読めるようになっている必要がある。

あなたがかいたCodeが横に長かった場合、横スクロールを強制してしまう。

80文字以内にする理由は多くある。リンク先のブログを参照すると良い。

4. フォーマットの変更を避ける

フォーマットを変えたい衝動に駆られるかもしれない。でもそれはやめてほしい。
フォーマットを変更することですべての差分に出てしまう
スペースの差分を出さないオプションをあるが、それをレビューアーが無視するには限界があるんだ。
特にCodeを移動した場合、それを無視することができない。やめてほしい。


スペースの変更がしたい場合、
ファイル内の前後のCodeを移動、フォーマットの変更、スタイルの変更を意図した別のPullrequestにしてほしい
そしてその主旨をコメント書いてほしい。

5. ビルドの確認をする

Pull requestする前に、ローカルでビルドの確認してほしい。
ローカルで動かないのであれば、多分他のマシンでも動かないんだ。

コンパイラの警告は注意してみてほしい。
もしかすると、あなたが気づかないうちにコンパイル警告によって、
レビューアーを妨げるかもしれない。

あまりりの多くのコンパイラの警告がある場合、

あなたのPull requestを拒否するかもしれない。

もしビルド用スクリプトがあれば、試してほしい。
そしてビルドが成功したやつのみPull requestを起こって欲しい。

私の最も多くのOpenSourceのプロジェクトでは、警告をエラーとして扱うビルドスクリプトを作っている。
このようなビルドスクリプトは自動かまたはさまざまな特定なCodeにおいてルールを実装している。

Pull requestを送る前に使ってほしい。
なぜなら、あなたのブランチをマージする前にも利用するからだ。

6. すべてのテストが通ること

テストコードが書かれている場合、
一度すべてテストを実行してほしい。

これは言われる前にするべきだ。
でも、私のところにはひとつまたはそれ以上でテストが通っていないPullrequestが届くんだ。。

7. テストの追加

また、すでにいくつかユニットテストが書かれている場合において、
あなたのPullrequestをテストCodeを追加しよう

私のところにテストがないPullrequestは余り届かないが、
しばしば私は拒否する。

これはハードなルールではない。
テストのカバレッジなしの必要ないろいろな種類のケースが存在する。

でもテストができるものは、テストをするべきなんだ。
そして、あなたは、既存のコードベースのテスト戦略に従うべきなんだ。

8. 理由をドキュメント化する

ドキュメント化したCodeはまれである。
「Codeにあるコメントは謝罪である」というが、
そして、私はコメントより型や変数が良い名前を好む。

まだCodeを書いている時、
私は自明でないかどうかしばしば意思決定をしなければならない。
(特にビジネスロジックの取引を行うときなど)

何を」書いたのかではなく「なぜ」あなたがそうCodeに書いたかを文書化する方法を

私の望ましい順に書いていく。

1. 文書化されたCode

あなたは、自己文書化されたCodeについて、意思決定することができる。
Clean Codeは、文字通りそれについてのHow-toが書かれている。

2. Codeにコメント

もし1が難しい場合、コメントを書こう
少なくとも、Codeと同じ場所にコメントを書こう。
バージョン管理システムが変わってもコメントが消えないようにする。
ここに、私がみたコメントで対処したほうが良い例がある。

internal static string[] GetSegmentsFrom(Uri href)
{
/* The assumption here is that the href argument is always going to
* be a relative URL. So far at least, that's consistent with how
* AtomEventStore works.
* However, the Segments property only works for absolute URLs. */
var fakeBase = new Uri("http://grean.com");
var absoluteHref = new Uri(fakeBase, href);
return absoluteHref.Segments;
}

3. コミットメッセージ

ほとんどのバージョンシステムではコミットを記述する機会がある。
そして多くの人が、必要最低限の記述のみとしている。
しかし、あなたはここに理由を書くことができるんだ。

幾つか、あなたは特定の順序でコミットを行わなければならず、

説明する必要があるかもしれない。
この順序については、Codeのコメントよりコミットメッセージにフィットする。

あなたが同じバージョン管理システムを利用する場合に限り、
あなたのこれらのコミットメッセージは保存される。

しかし、一度実際のCodeからファイルを削除、または、
別のバージョン管理システムを変更した場合メッセージが失われるだろう。

ここにたくさんCommitメッセージを書く必要があった例がある。

ただ、いつもそうであるとは限らない

4.Pull requestのコメント

まれに、上記の方法で適さない状況がある。
GitHubやStashを使っている場合に、Pull request自体にメッセージを追加することができる。
このメッセージは、Codeのコメント、コミットメッセージから更に離れている。
同じバージョン管理システムのホストを使用している限り保存されている。

例えばCodePlexからGitHubに移行すると、
Pull requestのメッセージが失われるだろう。

それでも、場合によってレビュアーに説明する必要があるだろう。
ただし、ソースコード外での説明が必要な場合である。
この方法が適しているがある。

あなたは明らかなものについては説明する必要がない。
しかし、慎重に考えてほしい。

今日のあなたが明らかだとしても、他人が明らかだと限りらない。
まして、三ヶ月後の自分が明らかであるとは限らない

9. 上手く書く

良いコードを書くことだけではなく、良い文も書こう。
これは主観的である。しかし、文とCodeの両方にルールがある。
Codeには正しいルールがある。そうでなければコンパイルはできない。
(インタープリタの場合、落ちるんだ。)

それはPull requestのメッセージ、コミットのメッセージ、Codeのコメントも同じなんだ。

正しいスペル、文法、句読点を使ってほしい。
そうじゃなかったらあなたの文を理解するのは難しい。

そしてあなたのレビュアーも人間なんだ。

10. スラッシングを避ける

レビュアーはPull requestについて、様々な点において指摘をする。
そしてあなたはそれに対して同意するだろう。

これは、Pull Requestのブランチに多くのコメントを残す原因となるだろう。
それ自体には何も問題はありません。

しかしながら、これは不当なスラッシングにつながる。

例えば、
あなたがA,B,C,D,Eの5つのコミットをしたPull requestをして、
レビュアーがBとCのコミットが好きじゃない場合に、Codeを消してと要求してきた。

ほとんどの人はPull requestのブランチをチェックアウトしている。
そして、問題のあるCodeを削除している。
つまり、A,B,C,Dと続き別のコミットFをしている。

なぜ、不要なCodeを追加して、また削除するコミットを追加するのだろうか?

これがスラッシングである。
つまり、無意味なcommitは何も提供しない価値である。

代わりに、該当のコミットを削除して、
強制的に、A,D,EのコミットをPushするようにしよう。
レビュー中のブランチのオーナーはあなた自身なんだ。

他のスラッシングの例として、
古くなってきたPull requestに起きる。(長い議論で起こる)
この場合、Pull requestのブランチのオーナーが定期的にPull requestが通る用にMasterからマージして最新を保つんだ。

もう一度、なぜ私は、すべてのMasterのコミットのMargeの見なければならないのか?
あなたはPull requestのオーナーである。
Pull requestをrebaseして、強制的にPushしてほしい。
そうすれば過去のコミットはすべてキレイになるんだ。

 

※ ここでのスラッシングについて、翻訳ではcommit履歴を言及していました。私自身は、「いちいち無駄なcommitも確認をしなければならない」レビューアーの負荷についてスラッシングと言ってるのかな-と思ってました^^;

 

まとめ

あなたのPull requestは一人または多くの人がレビューする。
レビュアーに仕事をさせてはいけない。
あなたがレビュアーにより仕事をさせた場合、あなたのPull requestはリスクであり拒否されるだろう。