GitHubのPull Requestテンプレートに何を書いているか？

初めに

信長の野望にドハマリしている技術開発部のフロントエンドエンジニア・村上(@pipopotamasu)です。

新潟出身なので上杉家を選択してゲームを進める場合が多いです。

しばらく私の書く記事でapolloに関する話題が続いたので、今日は気分を変えてGitHubのPull Requestテンプレート(以下PRテンプレート)の話をしたいと思います。最近私が所属するチームのフロントエンドエンジニアが2名になり(それまでは私1人でした)、ちゃんとPR作らないとなあと考え始めたのがきっかけです。

GitHubのPRテンプレートについて

PRテンプレートの細かい説明については割愛しますが、GitHubでPR作成時のDescriptionエリアに初期表示させるテンプレートのことです。

おそらくGitHubを用いてプロダクト開発をしている多くの企業でPRテンプレートを使用されているのではないでしょうか？

今回はそのPRテンプレートに何を書くのかについてなのですが、その前にそもそも「なぜPRのDescriptionを書くのか」について整理する必要があります。

なぜPRのDescriptionを書くのか

私が会社の業務においてPRのDescriptionを書く目的は以下2つです。

レビュワーがレビューする上で必要な情報を渡すこと
未来の誰かがPRを見返す時に当時の状況がわかるようにするため

レビュワーがレビューする上で必要な情報を渡すこと

2人以上いるチームで相互レビューしている場合はほぼ100%この目的を持っているのではないでしょうか？

このPRはどういう背景で作られたのか、何が追加・変更・修正されているのか、懸念点や補足事項があるのならそれはなんなのか、などコードレビューを始める上で情報を提供し、よりスムーズかつ質の高いコードレビューを実現するためのものだと考えています。

未来の誰かがPRを見返す時に当時の状況がわかるようにするため

息の長いプロダクトだと開発期間が数年以上に及んだり、開発メンバーが入れ替わることがあります。その際に入れ替わった後の開発者がある実装に疑問を持っても、過去の開発者が内容を忘れてたりそもそもすでにいない可能性があったりします。

その時に当時の状況を知る手がかりの一つがPRのDescriptionになります。開発の背景、懸念・補足事項などが書かれていた場合、その疑問が解消される可能性があります。

上記2つは私が書く時の目的ですが、他にもシステム統制的に必要なことを書いたりや、開発フローが複雑な場合は開発フローを明示化するためのチェックリストを置いたりなど、人や組織によって理由は様々だと思います。

PRテンプレートに何をかいているか？

さて、ここからが本題です。

先述した目的のため、以下のようなPRテンプレートを作り運用しています。

## チケットURL

## 対応内容・対応背景・妥協点

## やったこと
このPR内でやったことを書く

## やってないこと
このPR内でやっていないことを書く

## UI before / after
UIや振る舞いが変わる場合はbefore / afterのスクショや動画を共有する

## テスト
テスト項目、テスト方法を書く

## レビュー観点
あくまで目安です。

- 想定通りに動作するか？
- より良いJS/TS/CSS/Reactの書き方はないか？
- より良い設計方法はないか？
- 他の部分と書き方・命名・ディレクトリ構成等が異なっていないか？
- 関数、コンポーネントの粒度は適切か？
- etc...

## 補足

上から順に説明していきます。

対応内容・対応背景・妥協点

(詳しい対応内容はだいたいチケットに記載してあるので)ここでは簡単なPR内で対応した内容や背景、そして実装する上で妥協した点を記載していきます。チケットやPRタイトルに記載してある内容は省いたり最小限にとどめ、ここで主に書きたい内容は以下の2つになります。

チケット内容とPRの橋渡し情報

チケット内容とPRの橋渡しという意味合いの補足情報を記載するようにしています。例えば、「まだ仕様が未確定のため〇〇の部分は仮実装」というようなJIRAチケットに書くほどでもないがレビューする上で必要になる情報などを書いたりしてます。

妥協点

実装した部分で妥協した点があれば記載します。本来はコード内にコメントで書ければいいのですが、それがやりづらいものなど(例えばUIの挙動に関することや、コード内の個別部分ではなく全体を見渡せる場所に書いたほうがいい内容など)を書く場所になります。

私の経験上このような背景情報、補足情報、妥協点は、何かしらの問題解決のため過去のPRを見返すとき非常に有用だったりします。なぜなら未来の問題は過去の不確実性や妥協が引き起こす場合が多いからです。

やったこと

そのPR内で行ったことの記載をしていきます。

上記の「対応内容・対応背景・妥協点」よりも細かい粒度で書いていきます。

例えば何かしらのコメント機能の実装を例にすると以下のような感じです。

コメント新規投稿機能の実装
ライブラリの新機能を使うためバージョンを上げた

ざっくりこのPR内で何をやったのか内容把握できるようにします。

特に大きな改修や新機能追加などでPRを分ける場合は、それぞれのPRで何をやっていて何をやっていないのかを、後述する「やっていないこと」と合わせて書くことで全体を把握しやすくなります。

やってないこと

そのPR内で行っていないことの記載をしていきます。

特にPR内を分けた場合、後続のPRで対応予定だがこのPRではやらないことを記載していきます。

例えば...

コメント新規投稿完了後のサクセスメッセージ表示
アニメーション実装
テスト実装

などです。

他にも変更内容が広範囲に渡るので別チケットに切り出して別途対応するというのもここに記載します。

UI before / after

UI部分に変更が入る場合、そのbefore / afterのスクショなり動画なりをとり表示の変化や振る舞いの変化を共有するようにしています。

これをやる主な理由としては以下の2つです。

レビュワーの理解を早める

当たり前ですが、コードをレビューする際にその部分の振る舞いを事前に把握しているか把握していないかではコード理解のスピードに差がでます。特にフロントエンドは表示や振る舞いが視覚的にわかるものなのでその違いが顕著だと思います。そのため実装者がスクショや動画を取り、どういう表示や振る舞いを意図しているかを示す事によってレビュワーの実装意図やコード理解を早めることができます。

しばらくしてPRを見返した時、ローカルにブランチをチェックアウトせず当時のUIを確認することができる

開発をしていて「なんでこんな実装になっているんだろう」と疑問に思い、過去のコミットログ、そしてPRを追いかけることがあると思います。その際に当時のUIを確認したい場合など、PRのブランチが古いとnode.jsのバージョンを入れ替えたり、npmパッケージをインストールし直したりで非常に手間がかかります。そのためPRにUIの情報が視覚的に存在すると、スムーズに当時の状況を調べることができます。

テスト

私の所属するチームではレビュワーもローカル環境にてテストを行うので、テスト項目やテスト手順を書く欄を設けています。私の所属するチームではQAがいないため、PMがメインにテストを行っています。その負荷を少しでも下げるためレビュワーもテストをするようにしています。また、実際にテストをしてアプリケーションを動かさないと細かいインタラクションやCSS周りのフィードバックができないというのも理由の一つです。