dbt を使ったデータ分析プロジェクトのコードレビュー方針

背景

dbt を使ったデータ分析のプロジェクトを進めている。

チームメンバーが増えて、コードレビューをお願いするようになった。 メンバーによって、コードレビューのやり方がバラバラだったので、方針を決めて共有しました。

コードレビューの目的

信頼性の高いデータを早く算出できる状態を目指す。

• 信頼性の高いデータを算出する
• 可読性を上げて、算出するスピードを上げる
• 他の人のクエリを読んで、書き方を学ぶ

レビュイー

全て対応する必要はないが、対応していないところは明確にして後で対応したい。

• [MUST] と [ASK] 以外は、対応しなくても良い
  • 数行の修正など差分が少ない修正は、プルリクエスト内で対応する
    • 差分が多くなる場合は、別プルリクエストで対応する
  • 考慮漏れ なのか 考慮しているが対応していない のかわかるようにコメントを残す
    • 対応しなかったものは、Issue を作成してソースコードにリンクを貼り、別プルリクエストで対応する
• 実装に迷ったところなどは、レビュー前に GitHub の Issue にコメントを残して、レビュアーに相談する
• よくわからなかったところ・相談したいことなどがあれば
  • Issue やプルリクエストにコメントする
  • Slack のハドルミーティングで話す

レビュー依頼

• GitHub で Reviewers を設定する
  • Slack でのレビュー依頼は必要ない
    • 各自、GitHub と Slack を連携しておく
  • 期限が決まっている/急ぎであれば、いつまでにレビューしてほしいのか共有する
• 一度 LGTM をもらったら、下記の修正はレビュー無しでマージして良い
  • typo 修正、レビューされたとおりの修正など
  • 上記以外の変更は、再度レビューを依頼する

レビュアーのアサイン

2 人以上アサインする。

• typo 修正、モデルの削除など簡単な修正だったら、1人だけで良い

+

クエリ結果、ドキュメントなどを見てもらいたい場合は、依頼者もアサインする。

レビュアーを2人にしたい理由

• 複数人がレビューすることで、より信頼性の高いデータを算出できる
• 他の人のプルリクエストをレビューすることで、キャッチアップが進む
  • 他の人が何をやっているか理解できる
  • タスクの進め方、プルリクエストの分け方など参考にできる
  • クエリの書き方、モデルの分け方、アプリケーション側の実装、テーブル設計のキャッチアップが進む

プルリクエスト を作成した人がマージする

理由

• レビュアーがマージする場合、IMO のコメントをしたときにマージするべきか迷う
• レビュイーがレビューしてもらったあとに、微修正したい時がある
• プルリクエストを細かく分けたとき、プルリクエスト を出した人じゃないとマージする順番を間違えそう

レビュアー

気になったところは、すべてコメント/質問する。

• レビューを通して信頼性の高いデータを算出したいため
  • 定義/算出方法を間違えたまま、ずっとモニタリングされることになるのを避けたい
• 考慮/対応できていないところを可視化したいため
  • プルリクエストで対応できていないところを可視化して、今後の対応をどうするかまで決めておきたい
• 質問を積極的に行い、わからないところ/定義が曖昧なところを減らしたいため
  • レビュアーが、レビューしたモデルを参照して新しいデータを算出する機会が今後あるかもしれない
  • レビュアーがわからない場合、他の人もわからない可能性があるので、質問してより分かりやすいモデルに修正したほうが良い
• とりあえず LGTM、急ぎなので LGTM はやめる
  • わからないところは質問する
  • レビューに時間がかかりそう・レビューが遅くなりそうであれば、コメントする
    • レビューするプルリクエストが急ぎでなければ、レビューに時間がかかっても問題ない
  • 急ぎなら、マージ後にレビューし直す

レスポンス

レビュアーにアサインされてから、2 営業日以内にレビューを行う。 すぐにレビューできない場合は、レビュアーを変わってもらう。

レビューコメントのフォーマット

下記を参考にレビューしてもらえるとレビュイーが対応しやすそうです。

例

[IMO]

`not_null` のテストの追加をお願いします 🙏

[MUST]

このプルリクエストで、必ず対応してほしいもの。

• バグ
• 算出結果が正確でない
• 定義を間違えている

[IMO]

レビュアーから見て、自分だったら直すと思うもの。 対応するかどうかはレビュイーに任せる。

• リファクタリング
• 可読性向上
• 処理の変更
• モデルの切り分け方

[NITS]

些細な指摘、直したほうが良さそうな簡単な修正。

• typo 修正
• モデル名、カラム名の変更など
• クエリの書き方

[ASK]

よくわかっていないところは質問する。

• xxx は考慮/認識されていますか？
• 定義は xxx であっていますか？
• xxx の条件で絞り込んでいる理由はなんですか？

[GOOD]

良いところ、勉強になったところ。

• xxx の書き方がわかりやすくて良いですね！
• xxx の書き方が勉強になります！

[HELP]

相談したいこと。

• xxx で進めようと思っているのですが、問題ないでしょうか？
• xxx のモデルの yyy の定義を zzz に変えたいのですが、誰に相談すればよいでしょうか？

[SHARE]

共有したいこと。

• xxx は、対応に時間がかかりそうなので、#999 の Issue で対応しようと思います

参考

• コードレビューガイドラインを策定して継続的インテグレーションを実現する - Yahoo! JAPAN Tech Blog