うっかりな僕がプルリクエストを出す時に気をつけるべきことをリスト化してみた

うっかりな僕がプルリクエストを出す時に気をつけるべきことをリスト化してみました。

よくチームメイトにレビューしていただく中で以下のような指摘を受けることがあります。

 - typoあるよ
 - main(master)とコンフリクトしてるよ
 - プリントデバックのこってるよ

本質的なレビューの内容の前にケアレスミスで突き返されてしまう。

ちょっともったいないですよね。

これらを自分で気付ける方法はないかな？と思い、

「プルリクのレビュー前に気をつけるべきこと」 をまとめて自分でチェックするようにしました。

友人 から

世の中に共有されると有意義なコンテンツだ 

と意見いただいたので公開してみます。

また、友人が項目を足してくれてたりします。ありがとう

プルリクエストを出すときに確認すること

タイトルとコミット

• わかりやすい タイトル をつけよう
  • ストーリーとか目的がわかるように名付けよう
• コミットにprefixを付けてみよう
  • チームルールにもよるけど、prefix つけると目的がわかりやすい
  • 参考: https://qiita.com/numanomanu/items/45dd285b286a1f7280ed
• 不要なコミットは入れていないか？
  • 自分のローカル用だけの変更は入ってないか
  • コミットログを整理するなら git rebase -i を使う

Descriptionとコメント

• Descriptionを「初めてその仕事に触れた人が理解できる解像度」で書く
  • まずは関連issueを書こう
    • この仕事がissueを終わらせるPRなら closeを使おう
  • レビュアーが理解しやすいように関連するものを貼ろう
    • url系: slack , back log, チケット, 議事録
  • このPRでやったことを書こう
    • 意図的にやらなかったことを書くとよりgood
    • 特に見てほしい箇所や確認してほしい箇所があるなら書こう
      • 迷ってるところがあるなら書こう
      • 「A or B で迷ったが、〇〇の理由で B にした、大丈夫かな？」
  • 大きな変更は構成図があると分かりやすかったりする
  • レビュアーの人が動作チェックをやらなくてもいい工夫をしてみてもok
    • UI画面のPRならスクショ, gifで動きがわかるように
    • サーバなら実行結果や成果物を貼る、とか
  • 対象コードの付近に意図がコメントとして書かれているとレビューの負担が減る
    • 自信がない時はコードに対する違和感書くのも大事

ローカルブランチのプッシュ

• ローカルブランチを最新までpushしたかチェック
  • 「あ、最新もれてました！」はレビュアーにとって負担
  • そもそもそれは完成していない状態なのでmergeされちゃうとバグの原因になるよね

CIのステータス

• PRのCIステータスが完了 & 成功していることをチェック
  • main(master)とコンフリクトしてないだろうか？
  • そもそもテストコードは書いているだろうか？
    • あとから付け加えで書いてもいいけど、必要ならちゃんと書こうね
  • 失敗していたらビルドのエラー文はちゃんと読もう
    • そして15分調べてわからなかったら人に聞こう
    • 放置はダメ絶対

動作チェック

• もちろんPRの最新状態でビルドして動作チェックもする

コードの再確認

• コードを1行1行読んでみよう
  • 未使用のimport, 変数, 関数はないよね?
  • コードのタイポはないだろうか?
  • 不要なコメント、print debugの残骸はないよね？
  • 関数名、変数名の命名はそれでいいんだっけ？
    • $a など適当な変数名は残ってないかな?
  • GitHub にあげてはいけないものを含んでないかな？
    • Secret file, api tokenなど

プルリクエストのスコープ確認

• PRのサイズ(スコープ)は適切かな？
  • 複数の仕事を混ぜてないか確認する
    • リファクタリングと機能追加を混ぜてないかな？
  • File changeが大きすぎないかな?

新しくライブラリを入れる時

• ライブラリ入れたら、理由とgithubのURLを貼ろう

レビュアーの追加

• 適切なレビュアーを指定しよう
  • github なら Reviewers に指定しよう
  • いつもの人だけ？追加で見てもらったほうがいい人はいない？

以上です。

プルリクエストを出す時に気にするべきことは多い

プルリクエストを出すだけでも割と気にするべきことは多いですね。
こういうのは業務の中でだんだんと気づいていくものです。

明文化されずに気づいて行ったり、先輩に教えてもらったりして覚えていきますよね。

ですが、僕のように「知ってるけど考慮漏れ」が多い人はこういうリストに頼って、レビュー前に一回ずつチェックするように習慣づけた方がいいかもしれないですね。

僕自身もその用途で作ったので、皆さんも是非使ってみてください。

では。

special thx: @akki, @okashoi, kanufy