takatoku-room エンジニアになってからたくさんのPR(プルリクエスト)を作ってきました。
今では少しずつレビューをする側に周り、レビューしやすいPRとそうでないPRに違いがあることがわかってきたので言語化していきたいと思います。
駆け出しエンジニアやチーム開発の経験が浅い方に向けた内容になっていますので、少しでも参考になれば幸いです。
目次
- 動作確認をする
- PRの単位を小さくする
- 機能単位でわかりやすいコミットをする
- PRに基本的な情報を記述する
- コンフリクトは解消する
- 自分のコードにコメントを残す
- どのコミットでレビューに対応したのかを返信する
- 2回目以降のレビュー依頼には１回目からの変更範囲だけを切り取ったurlを添付する
- レビュー依頼をする際は規模感やスケジュールも一緒に伝える
以上を見て全部できてるわ！という方はこの先の内容はあまり意味がないので読み飛ばしてください。
逆に一つでも意識できていないところがある方は是非ここから先も読んでいただき実践してみてください。
いきなりプルリクの内容ではないのですが、これって本当に動くのか？を確認せずにレビューを依頼してしまうことは駆け出しエンジニアあるあるかなと思います。
私も仕様書をよく確認せず、大まかな実装だけやってレビューで指摘されることが過去に多々ありました。
レビュワーをやるようになってわかったのですが、レビューって意外と大変です。
また、人によって「コードレビュー」と捉えている範囲が違う現場が多いように思います。
例えば、動作するかを含めて「コードレビュー」と捉えている人がいる一方、コードレビューなのだからコードの可読性や拡張性、保守性などに特化してレビューしているという方がいたりします。
どんな方がレビューしてくれてもいいように最低限動くコードでレビュー依頼をして、効率的に仕事を進めていけるようにすると、手戻りが減りトータルでかかる工数は少なくります。
もし、実装方法が分からず質問したい場合や、ある部分だけ先にレビューしてほしいという場合には Draft Pull Requestにしてレビュワーに共有しておくと後々の手戻りを少なくすることができます。
個人差はあるでしょうが、私は、レビュワーにとって一番精神的ダメージが大きいレビュー依頼は、事前共有もなしに100ファイル以上の変更履歴があるPRを依頼されることだと思っています。
PRを小さくするためのアプローチはさまざまありますが、個人で明日からでも実践できる考え方を紹介できればと思います。
関心ごとにPRを区切ることで、レビュワーもその関心ごとに特化したレビューに力を入れることができるため、質の高いレビューを受けることができます。
関心ごとの単位として考えられるのは以下のようなものです。
- フロントエンドとバックエンド
- 機能要件と非機能要件
- Model View Controllerなどのレイヤーごと
- バグ修正
- リファクタリング
- テストコード
上記はあくまで一例であり、採用している設計手法やフレームワーク、要件によって適切な単位があります。
関心ごとを意識しながら設計、実装することで自ずとコードが疎結合になります。
逆にいうと、PRを分割できないような設計や実装になっている場合は、より良い実装にできないかを考える良い機会になるかもしれません。
個人開発や勉強などでは、コミットは作業ログのようなもので、切り方などは意識しないことが大半だと思います。
しかし、集団で開発する際にはコミットの切り方は大切です。
以下のようなコミットログを見てください。
- 投稿機能と投稿削除機能を実装
- 〇〇ファイル修正
- デザインfix
- バグfix
- レビュー指摘対応
これは全て悪いコミットの切り方とメッセージです。
例えば「投稿機能と投稿削除機能を実装」という複数の機能をまとめてコミットしていると、レビュワーはこのコミットをみて、これは投稿機能のためのコード修正なのか、削除機能のためのコード修正なのかわかりません。
しかしこれが「投稿機能を実装」と「投稿削除機能を実装」と機能単位でコミットが分かれていると各コミットがどの機能に対するものか容易に判断することができます。
その他のコミットはコメントの方に問題があります。
「〇〇fix」、「レビュー指摘対応」のようなコメントでは具体的にコミット内で何をしたのかわかりません。
これではコミットを分けてコメントしている意味がなくなってしまいます。
コメントも具体的にどのような目的に対する修正なのかわかるようにコメントを残しておくと後から見た人がわかりやすいです。
以上を反映すると上記のコミットは以下のようになります。
- 投稿機能を実装
- 投稿削除機能を実装
- モーダルデザイン修正
- 文言だしわけ処理のバグを修正
- hogehogeメソッドの名称をcoolNameメソッドに変更
だいぶコミットメッセージから内容が分かりやすくなったと思います。
コミットをわかりやすく切るのはレビューワーのためにもなりますが、調査で後からプルリクを見にきたエンジニアのためにもなります。
このコードはいつ、何のために実装されたコードかわかるようにしておくと後から見た人でも理解しやすくなり、チームの生産性を向上させることができます。
プルリクエストを作るときに、コメントを入力する欄があると思います。
ここには、最低限以下を書くようにしましょう。
- 目的
- 実装の大まかな説明
- 特にレビューしてほしいところ
- hogeメソッドのより良い実装はないか？
- ファイルの切り方は正しいか？
- etc..
- リリース日などのスケジュール
- その他関係する資料やPRのリンク
これらを書けばレビュワーは何のためにこのPRが作られたのか理解でき、どこを重点的にレビューすれば良いのかわかるため、すぐにレビューに取り掛かることができます。
ここではマークダウン方式で記述することができます。
githubで使えるマークダウンのチートシートを作ってくれている方がいるのでそれを参考に自分なりのテンプレートを作っておくと良いかもしれません。
まだチーム内でプルリクエストのテンプレートがない場合は以下を参考にテンプレートを作成してみると周りの人から感謝されるかもしれません。
集団開発をしていると度々発生するのがコンフリクトです。
コンフリクトは、違うエンジニアが同じファイルで作業していて、それが自分より先にmainブランチにマージされてしまった場合に発生するものです。
例えば、feature1ブランチで文字の大きさを6px→10pxに修正する変更をしたブランチをmainブランチにマージしたいとします。
しかし、feature2ブランチでは文字の大きさを6px→8pxへ修正していました。
先にfeature2ブランチがmainブランチにマージされた場合にfeature1ブランチで不整合が起こってしまいます。
あれ6px→10pxへの変更のはずが元が8pxになってるぞ？となるわけです。
コンフリクトを解消する手段はたくさんありますが、大抵最新のmainブランチをpullして差分を解消すれば大体のコンフリクトは解消します。
その他github上でコンフリクトを解消する方法もあります。
レビューを依頼する前に、一度最新のmainブランチを取り込んでおくか、プルリクを作ってコンフリクトが起こっていないか確認する癖をつけておくと無駄なコミュニケーションが発生することを未然に防ぐことができます
自分の実装で不安なところ、意見を求めたいところなどがあれば積極的にそのことをコメントに残しましょう。
コメントの残し方は公式ドキュメントで詳しく説明されているのでそちらをご覧ください。
ここでは、自分の不安なところだけでなくレビュワーが迷いそうなところを先に記述しておくことも大切です。
例えば、「本来この処理は〇〇ファイルに記述するべき処理ですが、〜の理由でこの場所に処理を置いています。」というコメントを残した場合には、自分の思考が正確に相手に伝わりコミュニケーションの手間が減ります。
このように自分の思考をコメントに残すことでレビュワーの判断の材料になりますし、より高いレベルでコードについて話し合うことができます。
また、レビュワー以外のエンジニアがPRを見た際にどのような理由でこのコードが作られたのかということが一眼でわかるため、リファクタリングなどでコードに手を加えて良いのか判断するための材料にもなります。
レビューをされたプルリクにはレビュワーからのコメントがつくと思います。
普通はレビュー後に修正すべきことがあった際には新たに修正してコミットしてから、再度レビュー依頼を投げるでしょう。
そして、指摘されたコメントのスレッドで「対応しました」などとコメントを送信していると思います。
しかし、そのメッセージに対応したコミットのコミットidを追記しておくだけでレビュワーはだいぶ楽になります
コミットidはCommitsタブから確認できます。
このようにレビュワーのコメントに対して対応したコミットidを追記しておくだけでレビュワーはほんというにその修正がなされたのかを判断するのが楽になりますし、自分でも対応漏れに気づくことができるので実際にやってみるの習慣をつけることをお勧めします。
レビューの際には何度も実装者とレビュワーの間で行ったり来たりすると思います。
その中で２回目以降もう一度レビューを依頼するときにはどうお願いしているでしょうか？
二日目以降のレビューをお願いする際には、前回のコードと今回の修正の差分をレビュワーさんが分かりやすくしておくと良いでしょう。
そのためには特定のコミットからの差分を表示するurlの生成する必要があります。
その方法は主に2つです。
- 「Files changed」タブを選択
- 「changes from all commits」を選択
- 変更部分のコミットをshiftキーを押しながらクリックして複数選択する
- 「Commits」タブを選択
- 変更前のコミットidと変更後のコミットidをコピー
- https://github.com/{ユーザー名}/{ブランチ名}/compare/{コミットid}..{コミットid}
上記のどちらかの方法でurlを生成したら、レビューを依頼の際やPR上のコメントにそのurlを残しておきましょう。
これをやるだけで、レビュワーの手間が減り効率良くレビューを進めることができます。
昨今はリモートワークが普及して非同期でのコミュニケーションが増えてきたと思います。
特にエンジニアは在宅で作業をすることが増えて、対面で同僚と話をするよりもチャットで話をする機会の方が多い方もいるのではないでしょうか？
そんな状況下ではレビュー依頼する場面でも、自分の所属するチームやグループに対して
「このPRのレビューお願いします。」
とお願いして、誰かが拾ってくれるまで待つというのが一連の流れかと思います。
しかし、「レビューお願いします。」だけだと意外とレビューをお願いされた側は拾いにくいです。
量はどれくらいだろう？
自分のスケジュール的に対応可能か？
あまり得意ではない技術領域の内容ではないか？
どのくらいのスケジュール間の内容なのか？
など、様々な不安要素から「はい！見ます！」とすぐに返事できないのです。
そこでレビューを依頼する際には最低でも以下の3つを伝えるようにしてみてください。
- なんのPRか？
- トップページデザイン改修
- 〇〇のためのapiエンドポイント追加
- 規模感はどのくらいで、どんな技術を使った変更か？
- バグ改修のためにメソッドをひとつ追加しました
- デザイン改修のためにcssを修正したフロント案件です
- スケジュール感はどのくらいか？
- 7/15にリリースしたいので、7/10までにはコードをfixしたいです。
最低限上記の3つを伝えることができれば、依頼された側は自分でレビューできそうかの判断をすることができます。
ここまで私が考える「【駆け出しエンジニア向け】レビューしやすいPRを作るために最低限意識すること」を説明してきました。
注意してほしいの、あくまでこれはPRを作る上で最低限の注意事項であることです。
上記以外でも意識できそうなところがあったら、レビュワーさん、チームのためにも良いプルリクを作ってみてください。(そして私にも教えてください)
