プルリクエストの実装中からマージするまでに気を付けていること

前置き

GitHubを使ったチーム開発をしていることを想定しています。
なんらかのバージョン管理ツールを使っていれば、適用できる内容かなと思います。
個人開発でも、多少手を抜くところはあっても大まかな流れは同じにしてます。

気を付けていること

自己レビュー的にやっていることが多くあります。
それでもレビューでコメントもらうことも多いですが、ミスはだいぶ減らせるのかなと思います。

実装

実装前

• おおまかな修正箇所を特定して、どこから手を付けるかを考える
  • コミットをどう分けると分かりやすいかも少し考えます
  • 既存のテストコードがどのくらいあるかも気にします
  • 対象リポジトリ以外にも影響がないかなど影響確認もします
• 既存コードが入り組んでいたら、まずリファクタできないかを考える
  • 仕様変更など新しいことを入れてからリファクタするよりも、まずきれいにした方がたいてい楽です
  • テストコードが不足していたら、まず現行仕様のままテストコードを追加します
  • リファクタだけで先にプルリクエスト作ることもあります
• どの順番・粒度でリリースすればより安全かを考える
  • ちょっとした修正であれば修正PR1つをただデプロイするだけで済むことも多いです
  • フロントエンドとバックエンドが分かれていたら、まずはバックエンドだけをリリースしたり
  • 既存機能ならまずはテストとリファクタだけやって、本当の機能改修だけ別にしたり
  • 改修規模が大きくなるほど、どうリリースしていくのかを考えて開発手順を考えるといいと思います

実装中

• IDEの警告にできるだけ対処する
  • 外部APIのプロパティ名など対処できないものは仕方がなく諦めますが、できるだけ適切な書き方に修正します
  • プロジェクトルールがあれば、チームで設定を共有しておくとよいでしょう(例: テストメソッドは分かりやすさ優先で日本語で書いてOK)
  • なぜ対処するのかも調べて自分なりに納得することで、次の修正が楽になります
• 実装方針に迷ったらひとまずモック的に書いてみる
  • ペアプロやチームメイトへの相談をする元手になります
  • 文章や図でうまく説明できるのであればそれでも問題ありません
  • 名づけに迷ったときも仮名でいったん書いてから、役割にあった名づけになっているかを検討します
• 修正した内容を確認する
  • コミットする前にどんな修正をしたのか差分を見ます
  • git diffでも見ることができますが、IDEだとGUIでサジェスト付きで差分表示できるので便利です(拡張機能が必要なIDEもあります)
  • 余計な変更をしていないか、タイポなどケアレスミスがないかなどを確認します
• 適切なまとまりでコミットする
  • 一定のまとまりでコミットをするとレビューがしやすいです
  • つい勢いに乗って修正しきってしまわないように気を付けます
  • 複数ファイル修正したとき意味合いが違うなら、git add .せずに分けてコミットします
• 不安なときはコミット履歴から改めて差分を確認する
  • コミット後にタイポに気づくこともあります
  • そっと修正してgit commit --amendで先ほどのコミットに入れ込むことも多いです(適切なまとまりでコミットしたい)
  • 小さい修正が積み重なったときは諦めて新しくコミットしてます

push時

• 未pushのコミット履歴の差分を確認する
  • また差分確認です
  • さっきまでは1コミット単位で見ていましたが、今度はまとめて見ることで変な修正になってないかを振り返ります
  • たいていここでコメントの修正忘れなどに気が付きます
  • コミットメッセージも変なところがないか見ます
• 作業端末が突然使えなくなっても困らないように一定間隔でpushする
  • まだ修正が残っていてもタイポが残っていて気になっても、翌日に持ち越すのであれば諦めてpushして帰ります
  • 突然休まなければいかないこともあるので、チームの進捗を止めないためにもできるだけ帰る前にはpushします
• ブランチ名を確認してpushする
  • 変な名前でブランチ作成していることもあるので、push前に修正します
  • 修正中に本当の要件に気づいた結果、もとのブランチ名が嘘になっていることもあります
  • ブランチ名はそこまで重要じゃないことも多いので、ここはほどほどにやってます

プルリクエスト作成

プルリクエストを書く

• 完成していなければDraftで作成する
  • Draftのままではマージできなくなります
  • アイコンも違いますし、途中だということが明確になります
  • プルリクエストを書くことで考えが整理されたりするので、先にとりあえず書いてみるのも良いです
• 何の修正をしたのかを明記する
  • issueと一対一関係であればissueへのリンクがあれば十分かもしれません
  • プルリクエストを分割していたら、どの部分の修正なのか分かるようにする必要があります
• どんな稼働確認をしたのかを明示する
  • テストコード以外で稼働確認していればキャプチャや実行結果を残します
  • 画面デザインに変化があればキャプチャもあると良いでしょう
• レビュアーに対してのメッセージを書く
  • コードコメントに残すほどではないけどレビュー時や後日見たときにあると嬉しい情報があればプルリクエストにコメントします(例: 参考にしたURLや公式ドキュメント)
  • 解消できなかった迷いや諦めがあればそれも書きます、アイディアをもらえるかもしれません
  • やったことだけでなく、やらなかったことがあれば明記しておきます

diff再確認

• プルリクエストの変更内容をブラウザで確認する
  • またまた差分確認です
  • IDEとは見え方が少し違うので、見落としに気づけることがあります
  • Windowsの人は、権限や改行コードなど余計な変更をしてないかも確認します
• 可能なら少し時間を明けてから再チェックする
  • 定時頃に完成したなら翌朝に自分でレビューします
  • 昼休みや別業務が入っていればそのあとでも
  • とにかく実装中の頭が抜けた頃に見直すことが重要です
  • ただプルリクエストのレビューまでに間を空けすぎるのも良くないので、ほどほどでレビュー依頼を出します

status checkを通す

• CIを成功させる
  • 修正してないテストコードで思いがけず失敗することがあるため、自動実行が終わるのを待ってからレビュー依頼しています
  • CI実行頻度が低いリポジトリだと本当に時限的に失敗することがあります(こないだあった)
• マージ先が進んでいたら取り込む
  • ぜんぜん関係ない修正だったらあとでまとめてやることもあります
  • たまに影響することもあるので差分が多いときはあらかじめ取り込むか、よっぽど差分があるときはブランチ切りなおしてcherry-pickすることもあります

マージ時

• マージコミットのメッセージが適切か考える
  • 1コミットだとプルリクエストタイトルではなくコミットメッセージがそのまま設定されたりします
  • あとで見て分かりにくそうであれば修正します
• リリースタイミングにあっているか検討する
  • 並行して修正していたり、週末だったり、今本当にマージして大丈夫か考えます
  • ここはブランチ戦略によっては、気にする必要がないこともあるでしょう

番外編

それぞれに何を書くか

t_wada氏のツイートをよく思い出します。

他の人はどんなプルリクエストを作っているか

Gitログをよく見ています。
プルリクエスト内容やレビューコメントなどを見て、
・こういう修正をしているんだな
・なるほどこういうレビューもあるのか
・この情報が書いてあって助かるな
など気づくことがあります。

たまに他の人のプルリクエストを覗くのは、息抜きにも勉強にもなるのでおすすめです。

うかつにリモートリポジトリを操作しない

GitHubアカウントではSSH認証を使っています。
SSHキー生成時にパスフレーズを設定することで、リモートリポジトリへの操作であればパスフレーズが必要になり、ローカルリポジトリへの操作ではないことが分かりやすくなります。
パスフレーズ入力をキャンセルすれば、リモートへの操作もキャンセルできるのでうっかりを防ぐことができます。

会社端末と私用端末でパスフレーズを分けることで、今どの端末で作業しているのかも自覚することができます

後書き

こうやって書いてみると、やっていることは意外と多かったです。
ここまでしなくても問題ない人もいると思いますが、ミスが多いなと感じていたら参考になるかもしれません。

薄々気づいていましたが「諦める」ということをよく言います。
今できなくても将来できることもあるし、自分ではできなくてもレビューやペア作業でアイディアをもらえることもあります。
適切な諦めは大事だなと思います。

参考になる他の記事

他にもプルリクエストやレビューの観点で多くの記事が書かれています。
以下3つは私もそういうことをしているなと感じた記事で、これを書くきっかけにもなりました。

追記

(2022/10) 実装前の項目にリリースについてを追記。全体的に細かい文言を修正。