巨大 Pull Requestの問題
- レビューに時間がかかる、疎かになる
- テストとコードを照らし合わせが大変
- 本番で問題が起きたときに、問題の切り分けがしにくい
- masterとの差分を反映する際のレビューが難しくなる
- 一つのミスが大きなfeatureブランチになるきっかけになることも
- 大きくなるとmergeに時間がかかり、ますます大きくなる
どのようにして大きなPRを避けるか
PRが大きくなりそうな時は事前に相談する
- 作業しているときに、変更点がたくさん必要になったときは早めにレビューアーに相談する
- 複数人で開発しているときは、しっかり話す時間をとる
- PRが大きくなる問題は後で変更するのが難しいので、最優先で相談するとよい。近くにいるなら直接相談する。いないときはチャットで相談する。
モデルだけの変更とテストを書く
- なにか機能の変更がある時に、まずモデルからレビューする
- 実際にどういう変化があるのか一番分かりやすいため
- モデルは、変更した箇所以外からも参照されることが多く、差分だけでなく、コード全体を見る必要がある
- なにを変更したのか、それに対してどのようなテストを書いたかをPRのdescriptionに書くとより分かりやすい
- viewやcontrollerのコードを含めないことで、本番への影響が少なく、レビューもしやすくなる
- 既存のscopeやmethodを変更する場合は、自分でもなにが影響しているか必ずみる方がいい
似たような操作で既存のコードをたくさん変更する場合は、それだけのPRを作る
- 似た操作をたくさんのファイルに対して行うコードと他の変更が混ざるとレビューが非常に困難になる
- レビューする際に確認することは、変更がすべてに適応されているのか。それぞれ同じように変更しても大丈夫なのかを見ている。
- スコープのデフォルトを変更するなど、1つの操作で影響が大きい場合も一つのPRにするほうがよい
- よく使う文言を変更する場合も、変更箇所が増えやすくできるだけ別PRにする
作業中にバグを見つけたとき
- バグ専用の別のPRを用意する
- 基本は、バグのPRがmergeされた後に、作業用のブランチに取り込む
- バグが作業に関係のある場合は、プライオリティをあげてレビューしてもらう
- バグが作業に関係のない場合は、バグを作業用のブランチに取り込まない
コードの記法などの間違いを直したいとき
- 記法変更のためのPRを作る
- 同じように間違えている箇所をまとめて治すのがよい
- コード記法についての議論と作業の議論が同じ場所で議論することになる、作業の内容以外の理由でmergeできない可能性もある
動作は変わらないが、既存のコードに影響がある場合
- 見た目の操作は同じだが、ある条件を含んだときのみに表示させる
- 例えば、もともとはただ投稿するだけだったが、下書き機能を追加する場合。下書きを機能(viewとcontroller)は実装していないため
- 新しく作られた状態のモデルが生成されても、同じように動作するか確認するテストがあるとベスト
Viewの変更を出す場合
- 新しい機能だけのViewの場合は、動線を用意せずにリリースする(viewの改善を詰めやすいし、必ず詰めることになるから)
- mergeする際に、既存のコードへの影響が減るメリットもある
- 新しい機能を本番環境で使ってみることができる、パフォーマンスの問題や社外の人に使ってもらうこともやりやすい
外部サービスや新しいサーバーを利用する場合
- 外部サービスへの接続とそれを利用するクラスの変更のみでまとめるのがよい
- レビューする際に、問題を分離出来る、確認項目もしやすい
フラグをつかって機能の出し分けをする
- 動線を無くすことが難しい場合に本番環境には影響がでないように、隠しオプションを利用する
- 影響が大きい機能リリースの時は利用する
最後に
解答が難しいテーマですが、自分だったらどのようなことを考えてどうアプローチするかまとめてみました。
特に、自分だけの問題でなく、チーム全体の問題なので、「事前に相談する」ってのは大切だと思いました。