Pull Requestを出してレビューしてもらってから反映。
どこにでもあるありふれた開発フローに付きまとう、どこにでもあるありふれた問題。
「Pull Requestがレビューされない」
もちろん開発フローにレビューが含まれている以上、レビューをしないメンバーにも非がないとは言えませんが、多くの場合はレビューされないPRには問題があるものです。
デカい
兎にも角にもデカいPRは読むのがつらいです。
もちろん要件が明記されていないなど、他にもPRが読みにくくなる原因はたくさんありますが、一番はこれです。
極端な話、1行変更のPRは他に何も書かれていなくても実装内容を察することができますが、10ファイル100行の差分と箇条書き20点の要件が書かれたPRは内容を把握するだけで一苦労です。
しかし、このこと自体は数カ月でもコードを書いていれば自然と勘づくもの。
問題はなぜPRが大きくなってしまうかということにあります。
Issueの単位とPRの単位は違う
当たり前の話ですが、Issue = PRではないため、Issueが担うべき責務をPRが担うべきではありません。
ある日あなたは総合職のメンバーに相談をうけ、新たな機能を実装することが決まります。
そのメンバーと多くの議論を重ね、ときには無理難題を押し付けられながら新機能の仕様が決まりました。
さぁ、楽しい楽しい開発の時間です。
新しくbranchを切って、手元に整理された要件を満たす機能を実装していきます。
testも動作確認も完璧、remoteリポジトリにPushしてPRを作り、そこで要件を説明しましょう！
これがダメなのです。
あなたが実装した機能の全容は、PRではなくIssueに記載されるべきなのです。
いくら自分の中では理路整然としていて、それほど大きな単位に感じないタスクであっても、それはあなたが多くの時間を仕様決定の議論に費やしてきたからです。
PRで初めてタスクを目にするメンバーにとっては、機能全容が一度に記載されたPRは理解が非常に困難であり、どのように分割すればレビューしやすいかすらもわかりません。
Issueは一つ以上(多くの場合は複数)のPR(branch)からなり、branchは一つ以上(多くの場合は複数の)commitからなります。
ここで、あなたがするべきことは、仕様が決定した瞬間にそれをIssueに書き起こすことです。
その後、Issueを分割するようにbranchを切っていくことで、PRの肥大化を防ぐことができます。
分割の単位については社内でルールが整備されていたりすることもあると思うのでここでは省きます。
参考: 巨大 Pull Requestを避けるための9つのポイント - Qiita
そのcommitはbranch名に寄与しているか？
名は体を表すというように、branch名は重要であり、PRの実現するべき機能を表すように命名するべきです。
たとえばブログ記事を投稿する機能を改修するようなPRを作ろうと考え、 refine_posting_article_function といったようなbranch名を付けました。
Viewを改修してcommit、次にControllerのロジックを変更してcommit……ついでに目についた規約に反しているコードも直してしまいましょう。
これがダメなのです。
最後のリファクタは、明らかに posting_article_function を refine することに寄与しません。
たとえ些細な変更であっても、無関係な差分は積もり積もってPRの理解に混乱を招きます。
こういった場合には、別のbranchを切ってPRを作ることで対処しましょう。
リリースのタイミングでPRを区切らない
上記のことを意識しつつも、いざbranchを切ろうとすると、同じタイミングでリリースする機能は同じbranchに乗せてしまいがちです。
しかし、Issue = PR ではないのと同様に、リリースのタイミングで区切られたPRが機能を上手く分割しているとは限りません。
これはどちらかといえばGitの取り回しの話になってきますが、個人的には同じタイミングでリリースしたい機能群については、それを乗せるためのbaseとなるようなPRを作り、そこから各機能を実装するためのbranchを切っています。
baseとなるbranchは機能をまとめてリリースするために作るものなので、空commitで作ってしまい、レビューの通ったPRから順次mergeし、最後にtestが通ったらリリースという流れです。
(この辺りはなけなしのGit力で考えた方法なので、他にいい方法がありそうなら是非ご教示ください。)
要件が書いていない・記載が雑
PRのサイズを落としたとして、やはり説明の不十分なPRは読みづらいです。
最低限
- 関連するIssue
- 要件の概要
- 変更点
- 想定される影響範囲
あたりは記載されているのが望ましいです。
Issueの方に仕様の全容が記載されてあれば、PRでの要件概要は案外少量で済むものです（逆に大量になるようならさらにPRを分割した方がよいです）。
また、変更箇所のスクリーンショットや動作中のgifなど、一目で何をやったかがわかるものが添付されているとよりよいです。
チーム内で記載するべき情報のルールを決めて、Pull Request templates を積極的に活用していきましょう。
testが書いていない
test codeの有無はレビューの負担にも直結します。
testコードを読めば、最低限コードで保証されている処理、不具合が起きない状況が把握できるため、重点的にレビューする点が把握しやすくなります。
testコードは積極的に書いていきましょう。
アピールが足りない
やることやっても読んでもらえなければとにかくお願いしまくりましょう。
レビューは仕事の一部なので遠慮する必要はありません。
逆にお願いされた側も無視したりしないで、PRが読みづらいなら改善の要望を出したり、今時間が取れないならその旨を伝えるなど、ちゃんとコミュニケーションを取りましょう。
コミュニケーションはすべての基本です。
まとめ
- PRのサイズは小さく保とう
- そのためにIssueとPRの関係性、PRとcommitの関係性を意識しよう
- 実装する機能の全体像をIssueに記して、それを分割・実装した機能についてPRに要件を書こう
- testコードを書こう
- チームメンバーと適切にコミュニケーションを取ろう