私が歌川です

@utgwkk が書いている

ふだんの開発でPRを出すときに考えていること

業務の話です。OSSとかだとまた変わってくるのかもしれないし、共通することもあるかもしれません。

  • 先に作戦を練る
    • 実装する前に、方針段階でレビューしてもらえるとよい
    • 自分だけでは気づけない考慮漏れとか、こういう方針もあるよっていう提案とか、いろいろ得られるものがある
    • 先に実装完成させてから、これでは要件を満たせない・うまくいかないねってなるともったいない
  • 巨大なPRにしない
    • diffの大きさについては、プログラミング言語とか、利用するフレームワークによっても変わってくるので、一概には言えなさそう
      • +1500, -1500 だけどスナップショットの更新があったとか
      • インデント1つ下げることになったとか
    • たとえば、あらゆる機能を1つのPRで実装してたら巨大なPRになると思う
      • 1つのPRであらゆるものを実装しない、1機能ずつ実装するとか、1つの層だけ実装する、とか
    • PRが巨大だと、コミュニケーションを取りつつ延々とレビューを打ち返す、みたいな感じになってよくない
      • お互いに疲れる
      • とくにパートタイムだと、少ない労働時間でずっとやり取りを続けることになる
    • DB→モデル→サービスメソッド→コントローラー→UI みたいな感じで、少なくとも層ごとにPRを分ける
      • 分け方は諸説ありそう
    • 1つの層の中でもちょっとずつやる
      • CRUD一気に、ではなく、まずはCから、とか
      • Cの実装とテストをまとめて1つのPRとして出して、レビューしてもらう
    • PRの大きさとは何か? という問いに対して、PRが向けている関心の多さがそれにあたるのではないか、というのを思ったけど、うまく言語化できてない
  • 文脈を書く
    • こうしました!! だけ書いてあっても、なぜこうしたいのか分からない
    • 最低限、issueへのリンクは貼っておきたい
  • 動作確認の方法を書く
    • 「どういう条件で」「何を」「どうすると」「どうなれば正しい」のかが分かるように書く
    • テストが通ればオッケーならそのように書いておく
  • PRのテンプレートに従う
    • それはそうですね
    • ちゃんとしたテンプレートができていると、上の2つはだいたい満たせそう
  • テストを書く
    • テストを見たら、こういう振る舞いが想定されている、というのが分かる
    • メソッドのインタフェースとか、使われ方とか分かる
    • 機能追加やリファクタリングするときに、既存の機能が壊れてないかどうか分かる
      • 影響範囲が見積もれる
      • 軽い修正のつもりがけっこう既存の実装が壊れる、とか分かる
    • 丁寧にテストする
      • 正常系・異常系・コーナーケース
      • 不等号とか間違えがちなので、どういう入力に対してどういう出力が想定されているか、ちゃんと書く
        • よく条件を逆にするので、自信ないと思ったらテストケースを足す方に倒してる*1
      • テスト書きすぎ問題 - hitode909の日記

追記

CL の作者がコードレビューを乗り切るためのガイド | eng-practices小さなCLのページに、巨大でない・小さいPRとはどういうものかについて言語化されているのを id:shiba_yu36 さんに教えてもらいました。自分が検討違いなことを言ってないかちょっと気にかけてたけど、これを読む限りでは間違ったことは言ってなさそうでした。

リファクタリングを先にやって差分が大きくならないようにしましょう、CL*2をどうしても小さくできないように見えるときも、検討に検討を重ねましょう、それでもCLが大きくなってしまうと分かったら十分に注意しましょう、ということが述べられていて同意があります。