この記事は LITALICO Engineers Advent Calendar 2021 7日目の記事です。
業務でPull Requestを出す際に気をつけていたり実施していることがいくらかあるのですが、あまりそれをアウトプットする機会はなかったので、ちょっくらまとめてみます。
前提として、社内でGitHubを使っている・数人程度のチームでwebサービスを継続的に開発している環境を想定しています。
また、Pull Requestの変更内容や目的の良し悪しの話題（その変更は必要なのか？目的に沿っているか？など）については本記事の対象外とします。
タイトル
タイトルは当然やったことを書きますが、「見る人にとって過不足なく識別できる情報量」を入れるようにします。だいたいサラッと一文、感覚的には助詞が1~2個に収まる程度。
ここで長くなりすぎるようであれば、タイトルには不要な（本文にだけあれば良い）情報が含まれているか、1 Pull Request内でやっている内容が多すぎる（分割すべき）というサインかなと思っています。
他との順序関係
また内容以外で情報を入れるとすれば、特定の順番でレビューしてほしい場合にそれとわかるような情報を入れると親切です。
例えば「ある機能の実装」と「その実装の前段としてリファクタを実施」という順序関係のあるPull Requestがある場合、「XXXの変更のための事前リファクタ」と「XXXの変更」のように関連と前後がわかるように命名します。
すると、Pull Requestのリストで見たときに「前者からレビューすれば良さそうだな」と判断できます。
似たパターンで、順序付きのものが複数ある場合には連番の番号を入れることもあります。
本番環境への影響
他にも、業務プロダクトに特有な要素として「本番環境への影響有無が誤解されないようにする」ことに気をつけます。
Pull Requestには通常の機能リリースの他に、ドキュメントやテストのみの変更で影響が発生し得ないもの、リファクタで挙動は変えないが影響の可能性はゼロではないもの、機能実装したが参照を切っていてユーザに見えないものなど、本番への影響度には色々あります。
これらについて「XXXに関するドキュメントを追加」「[未リリース]ZZZの機能の実装仕込み」など影響が察せられるようにタイトルを設定すると、他者から見て判断しやすいかと思います。
タイトルではなく本文に入れても良いのですが、タイトルにある方がPull Requestリストや通知などを見たときにわかりやすく、また不要な驚き（「これ今リリースしないんじゃないの！？」など）が無くせて良いと思います。
※筆者のチームではリリース時にPull Requestタイトルがslack通知される運用になっており、非エンジニアメンバーも見られる前提があるため、この点の重要度が高めになっている事情があります
本文
本文は変更の目的や設計意図など、コードでは語られない情報を置くのがベースかと思います。また関連issueや、mainでないbaseブランチがあればそのPull Requestなど、関連情報を辿れる情報も添えます。
また実装したものの意図だけでなく、検討したがやらなかったことも記しておきます。特に最終的な実装が素直なものでない場合はその理由が記されてないと、レビュアーに「なぜ素直なXXXの方法を採らなかったんだろう」と不要に悩ませてしまいます。
似た観点では、方針について特に悩んだ場合はその思考ルートを記しておくと良いなどがあります。
Draftで作る
Pull Requestを作成する際にはまずDraftで作成します。
公開してレビュー依頼をする前に、セルフレビューを実施しながらpullreq本文やコードの修正をするためです。
セルフレビュー以外の観点でも、Ready for reviewの状態でCIが落ちたり検証でバグが見つかったりすると、レビュアーにバグのあるコードを見せてしまうことになります。直すことが確定しているコードを見せるのは時間の無駄ですが、Draft状態であれば問題ありません。
また、一度もReady for reviewしていなければ、commit書き換えやrebaseによるforce pushも問題無いと思っています。むしろリリース頻度によってはReady for review直前にrebaseしたほうがconflictなどのリスクが下がることもありますし、また後のforce push想定で作業初期のうちからDraftで作成したほうが進捗が見えてよかったりします。
逆に言えば、Ready for reviewしたのであればforce pushは絶対に避けます。レビュアーのレビュー状況やコメントresolvedなどのステータスが全部消えます。最終的なコード差分が同じであろうが、レビュー中であれば絶対に避けます。
何かの都合によって本当に必要であれば、一旦draftにするなりして断りを入れます。
セルフレビュー
Draftで本文などをざっくり書き終わった後は、レビュアーに見てもらう前にまずセルフレビューをします。
やることや目的としては
- やったこと全体を再確認する
- 意図せぬミスや差分が入り込んでいないかチェックする
- 改善の余地や説明不足が無いかをレビュアー視点で確認する
- レビュー負荷が下がるように調整を行う
などです。
特にレビュアー負荷の軽減はレビュアーの人数の分だけ効いてくるため、多少手間でもトータルでは効率化に繋がることが多く、積極的に実施します。
セルフレビューがしっかりとできてから、Ready for reviewしてレビューを待ちます。
レビュー対応
特筆すべきコメントの無い一発Approveでない限りは質問への回答やコードの修正を行いますが、実際にリアクションをする前にちょっと考えます。
パッと一言で回答できるような質問が来た場合、「そもそもその質問が発生しないようにできなかったか」と考えます。
質問が発生したということはPull Requestに疑問の余地があったわけで、セルフレビュー時点で発見・対処できたかもしれません。もちろん深い議論はするのが良いですが、シンプルな質疑は減らせることが多く、次以降の改善点とします。
※「質問」としていますが「意見」でもだいたい同じです
またRequest changeであっても、その修正が必要だと思った理由をより深ぼることで更に良いコードにできる可能性があります。実例として
- 元コードに、ファイル内に同じ正規表現マッチをする処理が二箇所あった
- 「正規表現パターンを定数に切り出しては？」と指摘があった
場合に、パターンだけでなく、マッチ処理とその後の変換コードをまるごと関数に切り出したことがありました。「そのパターン、DRYにできるよね」という指摘を「そのマッチ周りの処理、DRYにできるよね」としたかたちです。
ただし、こちらは方向性を間違えると「そういうことではないのだが...」となりかねないので、ちょうどよい塩梅を探る必要はあります。
まとめ
なんとなくそれっぽくできてしまうPull Requestの一生ですが、それぞれの部分で結構考えられること・考えると良いことはあると思っています。
本記事はかなり個人的な考えではありますが、日々やっている開発の中で参考にできることがあれば幸いです。
Discussion