ふだんの開発でPRを出すときに考えていること - 私が歌川です の続編です。小さなPRを出す、という話がありましたが、どうやって小さくしているのか、というのを書きます。
どういうメソッドが欲しいか考える
- Aという情報をBという画面に出したい
- Aを取得するメソッドがないので、それを作る必要がある (メソッドCとする)
というときに、メソッドCを作るPRをまず先に出して、マージされたら続いてCを使って得られる情報Aを画面Bに出すPRを出す、というのをやります。 実装方針を考えて既存実装を眺めてからそういう作戦を立てることもあれば、実装しているうちにメソッドCが足りないな、と気づいて別ブランチでPRを作ってそちらを先に出す、ということもあります。
このとき、1つのPRで両方やろうとすると、メソッドCの実装のレビューと、情報Aを画面Bに出す実装のレビューが同じPRで行われることになります。メソッドCのロジックが小さいうちはなんとかなるかもしれませんが、込み入ったことをやろうとすると、レビュワーがレビューすべきこともレビュイーが修正することもどんどん多くなって大変です。
最初はシンプルなロジックだと思っていても、レビューを受けて修正を重ねていくと、だんだんしっかりしたロジックになってきたぞ、となることがあります。最初のPRがじゅうぶん小さければ、修正を重ねてもロジックは1つなのでまだ小さいPRですね、というふうにできます。
1つのPRでついでにやらない
実装が終わったあとに、既存実装部分をついでに修正したくなることがありますが、ぐっとこらえます。ここで「ついでに」と言ったのは、PRで実装したいロジックの本筋とは関係ない部分を指します。ついでにリファクタリングできるとリファクタリングができて嬉しいのですが、ロジックのレビューと、リファクタリング部分が既存ロジックを壊してないかのレビューが同時に行われることになります。
また、実装方針を考えたけど今のメソッドのインタフェースだと実現が難しいからリファクタリングしないといけない、という場合もあります。こういうときも、リファクタリングと機能実装を同時にやるのではなく、まずリファクタリングをやってPRを出して、やりたかった機能実装を次にやる、というふうに進めます。PRでは実装方針を伝えつつリファクタリングの理由を説明します。
一気に全てを良くしたくなる気持ちは分かりますが、ぐっとこらえて1つずつ解決しましょう。
分割方法が分からなかったら聞いてみる
慣れてない箇所や、馴染みのない概念に触れるときは、どうやって実装を分割するか見当がつかないことがあります。そういうときは、その箇所・概念について知ってる人に聞くのが手っ取り早いです。レビューで指摘された場合はレビュワーに聞いてみるのがよいでしょう。あるいはペアプロ・ペアレビューをするのもよさそうです。
おわりに
小さなPRに分けるために考えていることを紹介しました。こういう仕草をどこで習得したのかは忘れました。最初からこれができることを求められることはないだろうと思うので、経験を積むのがよいのではないかと思っています。機能を完成させることを目標として、完成のためのパーツを1つずつ作って組み合わせる、みたいな振る舞いかたをしている、というふうに自覚しています。
最後に、いい言葉があったので引用して記事を締めます。
小さすぎる CL を書いて失敗するほうが大きすぎる CL を書いて失敗するよりもよほど良いです。レビュアーと協力してどんなサイズの CL なら受け入れられるかを探ってみてください。
