こんなプルリクは嫌だ

Octicons-git-pull-request.svg.png

はじめに

アウローラでの開発はBitbucketで行われており、書いたコードをプロダクトへ取り込む際には基本的にプロダクト管理者のレビューが必須となっています。
コードレビューはコードの品質を上げたり、レビューイ(とレビュアー)のスタディにもなるしでいい事ずくめだけど。やーーーーーーーーーーーっぱり時間がかかるものです。
そこで、主に 私の開発速度を下げないため…もとい、チームの開発速度を下げないために、反面教師としてレビューしにくいPullRequestの紹介をします。

えー、これを書いたからにはこの内のどれかに引っかかるPullRequestが私に投げたら、ソッコーでrejectするからヨロシコ。
というわけでいってみよう。

マークダウン使ってない

一番最初に体裁のことを書くのもどうかと思うけど、githubやBitbucketではマークダウン形式で記載ができるので積極的に使う。
見出しや箇条書きで書くだけで、つらつら書くよりも相当見やすくなる。

私がよくやるのは、Macdownというマークダウンエディタを使って、バーっと書いた後に、PullRequestの画面に貼り付けるスタイル。 Bitbucketのテキストボックス部分はあまり使いやすいとは言えないので、手元のマークダウンエディタでやる形に落ち着いた。

なお、Macdownを使っているのはショートカットキーが使えるというただ一点なので、別にAtomでもPHPStormでもなんでも良いよ。

ちなみに個人的に使っているPullRequestのフォーマットは以下

タイトル

概要

やったことをさらっと完結に

詳細

以下を参照のこと。 課題(弊社ではBacklogへのリンク) - title

特記事項

実装上のコメントやBacklogで表現しきれないことを記載

コミットログ

PullRequest作成時、もしくはgit log で出力されたログを記載

マークダウンVer

# タイトル

## 概要
やったことをさらっと完結に

## 詳細
以下を参照のこと。

課題(弊社ではBacklogへのリンク)
- [title](url)

### 特記事項
実装上のコメントやBacklogで表現しきれないことを記載

## コミットログ
PullRequest作成時、もしくはgit log で出力されたログを記載

変更箇所多すぎ問題

1PullRequestに変更ファイルが1,000超えていたりするやつ。
通常の変更ではあんまりないけど、masterから空ブランチを切って、フレームワークをインストールしたりするとこうなる。

解決方法としては、フレームワークインストールは別にしてPullRequestを立てて、とっととマージしてしまう。(チームの方針にもよるけど、初期インストールとしてのフレームワークに関してはpushでも良いかもしれない) その後の開発については、マージ後のブランチから実施する。

今回いじってないけど変更に出ちゃいました問題

基本的に原因は2つ。

  1. PullRequestの投げ先の変更箇所が適切に自分のトピックブランチに反映されていない
  2. PullRequestの投げ先自体がトピックブランチの親ブランチで行われた変更が反映されていない。

タイミングによっては回避しづらいんだけど、以下の点は確認してほしいところ。

PullRequestの投げ先の変更箇所が適切に自分のトピックブランチに反映されていない

コミットログを確認して、自分がブランチを切ったよりも後に親ブランチが更新されていないか確認する。更新されていれば、トピックブランチに親の更新を取り込んで(マージして)必要ならpushしなおす。

PullRequestの投げ先自体がトピックブランチの親ブランチで行われた変更が反映されていない

こちらもコミットログを確認。トピックブランチの親ブランチと、PullRequestの宛先ブランチの差分を確認する。 ここに差分が出ている場合は、なんらかの理由(緊急対応、もしくは開発上の事情、または怠惰)があるので上司に確認しよう。 特にチーム開発で幾つものブランチが平行して走っていくような場合は、このようなことになりやすいので注意。

ignore系ファイルもプルリクに含まれている問題

本来はignoreすべきファイルがプルリクに含まれていて、かつデスクリプションで何も触れられていないパターン。
初期のリポジトリ構築時のミスにより含まれてしまったものなのか、それともなんらかの理由によりリポジトリ管理が必要になったものかわからない。
ぽけっとしていてmasterまでコミットしてしまうと、後でgitの管理下から逃れるのが面倒になるので適切にignoreされていない場合は適宜気がついたタイミングでignoreファイルの整備をすること。

ってこらそこ、ignoreの整備が面倒だからって言い訳して.ideaファイルをコミットすんじゃないよ、開くたびに変更検知されるじゃろがい。

放置されたコンフリクト問題

さすがにあんまりないと思うけど、プルリク投げてコンフリクトしてるけど放置するパターン。ただ、投げた当時はならなくてもしばらく経ってコンフリクトになるパターンもあるので、なかなか難しかったりもする。

コードフォーマット直しました系が含まれている問題

フォーマットを直した規模にもよるんだけど、ファイル全体のフォーマットがガッツリ直っていたりすると、プルリクが…というか確認すべきポイントが非常に見辛くなる。 歴史のあるプロダクトで途中でルールが変わったりすると、カッとなってやりたくなる事象。

個人的にはコードの変更をかける前にフォーマットをがっつり直したくなったら、それ専用のプルリクを作って欲しい。ソッコーマージするからw
それが難しい場合は、せめてコミットを分けること。フォーマットの変更と処理の変更のコミットがまとまっていると、bitbucketのプルリク確認画面での確認は困難になる。

本番リリース考えてません問題

安全にリリースするためのPullRequest、リポジトリ管理なのにリリースすることが考えられていないもの。
例えばデータベースの更新があるのに、スクリプトがないとか、あるけどたくさんあってしかも順番を考慮しないといけないようなイメージ。 この状態になっていると、レビュアーでできることはあんまりない(却下するぐらい、リリースできないから)

逆にどうなっていると嬉しいかって言うと、例えばデータベースの更新であれば…

  1. リリース時に実行するファイルが1まとまりになっており、上から実行すればよいもの。
  2. 複数ファイルに分かれているが、実行用のプロシージャが組まれているので、それを叩けば適切な順番で実行してくれるもの

1.2.どっちでもいいけど、このような感じになっていると非常に安心できる。

まとめ

と、いうわけでPullRequestを受け取る際にこんな形になっていると嫌なものをまとめました。程度の差こそあれ、どれも確認に手間取ったり、リリースの際に手間取ったり、そもそもリスキーだったりとプロダクトを安定して運営していくにおいて、小さな障害になりやすいものばかりです。
とは言え、どれもこれもPullRequestを投げる際に、ちょっと気をつけるだけでコミュニケーションが潤滑になるものも多いので、PullRequestを投げる際はぜひ、気をつけてみてください。

次回予告w

今回はPullRequestを受け取る側の目線ですが、次は逆です。題して…

こんなプルリクのコメントは嫌だ

PullRequest作成者から見て、(上司とは言え)こういうコメントされると腹立つわ〜というポイントを纏めていきます、ええ自戒も込めて。

乞うご期待。