初めに

チームの中では自分はコードレビューでコメントを良くする方ではあるので、最近ペアワークの中で一緒にチーム内の Pull Request のレビューなどをしながら、どんなところを見て、どんなことを考えてるか共有したりしてます。

ただ、この辺言語化できてないなぁ、いつかまとめてみたいなぁと思ったのでまとめてみる。

適当に思いつく感じで書いていくと、そのへんにある「コードレビューのすゝめ」のような抽象的な記事になってしまうと思ったので、どうせなら実際にここ最近で実際にしたレビューのコメントを元に書いてみようと思います。

※ 次の章はGitHub GraphQL APIで自分のPRレビューコメントを取得する話なので、技術的な話を飛ばしたい方は → 「どんなコメントを最近していたか？」にジャンプ

ここ1ヶ月くらいでどんなコメントしているか？

最近知った GitHub の GraphQL を利用して、Pull Request に対して自分がつけたコメントを取得してみる。

1. GitHub Explorer を使う

GitHub が用意している Explorer を使うと、かんたんに GitHub の GraphQL API を試すことができます。

ただし、コレ自体が GitHub の OAuthApp で、Private リポジトリを参照するには Organization の承認が必要なります。前は使えたのですが、今は許可制になった？みたいなので、許可してもらえないと残念ながら Private リポジトリについては見ることはできません。

2. Script を書く

Shell でも JavaScript でも Ruby でも何でもいいのですが、お手軽なスクリプトを書いてゴニョゴニョして取得することにしました。token xxx には自分のアクセストークンを設定します。

下のクエリだと、

自分が作成した
Issueへのコメント（Pull Request 含む）
最新100件

が取得できます。これで、PR そのものにつけたコメントを取得することができます。

.js

const { graphql } = require("@octokit/graphql");

(async function(){
    const graphqlWithAuth = graphql.defaults({
        headers: {
          authorization: `token xxx`,
        },
      });
      const repository = await graphqlWithAuth(`
      {
        viewer {
          issueComments(first: 100, orderBy: {field: UPDATED_AT, direction: DESC}) {
            nodes {
              repository {
                nameWithOwner
              }
              pullRequest {
                title
              }
              body
            }
          }
        }
      }`);
      console.log(JSON.stringify(repository, null, 2));
})();

コードに対するコメントの取得は以下のようなクエリを使いました。

特定のリポジトリ
直近100件の PullRequest
その PR の50件のレビュースレッド
そのスレッド内の10件のレビューコメント

を取得できます。

.js

      { 
        repository(owner: "XXX", name: "YYY") {
          pullRequests(first: 100, orderBy: {field: CREATED_AT, direction: DESC}) {
            edges {
              node {
                number
                title
                reviewThreads(first: 50) {
                  edges {
                    node {
                      comments(first: 10) {
                        edges {
                          node {
                            id
                            author {
                              login
                            }
                            body
                            createdAt
                          }
                        }
                      }
                    }
                  }
                }
              }
            }
          }
        }
      }

ただし、これは当然自分以外の人のコメントも含むので、ゴニョって自分の付けたコメントのみに絞ります。

レスポンスの構造が複雑なので、最終的に [PRタイトル、コメント日時、コメント] になるようにいろいろゴニョってみました。

.sh

$ node XXXX.js | \
  jq -r '
    .repository.pullRequests.edges[].node | 
    {title, comment: .reviewThreads.edges} | 
    {title, comment: .comment[].node.comments[]} | 
    {title, comment: .comment[].node} | 
    {title, author: .comment.author.login, comment: .comment.body, createdAt: .comment.createdAt} | 
    select(.author == "your_name") | 
    [.title, .createdAt, .comment] | @tsv' \
  > outputs/YYYY.csv

どんなコメントを最近していたか？

ようやくここで本題。とってみた情報をいろいろゴニョっと Excel にまとめて、それぞれのコメントを頑張ってカテゴライズしてみた結果、こんな感じになりました。

コメントの種類	Count
PRのDescriptionの充実化・レビュワーの負荷軽減	15
可読性・保守性	15
設定／ロジックの不足（Bug含む）	13
不要な設定／ロジック	11
PRのスコープ	10
原因の特定	6
その他	5
一貫性	4
Suggestion	3
テスト	2
仕様	1

それぞれどんなコメントだったか少しみていきたいと思います。
（結局具体的な背景とコメント内容がないと伝わらないことも多そうですが、とりあえず書き出してみます)

【1】 PR の Description の充実化・レビュワーの負荷軽減

このカテゴリは、おもに PR の Description に不足している内容を補う、または確認するためのコメントです。ひいては、今後 PR 作るときにはその辺書いてあるとレビュワー視点では嬉しいなぁという気持ちを伝えるためのコメントです。

Example

その PR が何かの変更を元に戻している場合、Revert 元の PR リンクをメモとしてコメントに追加
ある変更が別の場所の設定をコピーして修正を加えている場合、メモとしてコピー元との Diff をコメントに追加
変更箇所の整理とそれぞれの理由の確認するコメント
どのような経緯で今回の実装方針になったかの整理するためのコメント

【2】可読性・保守性

このカテゴリはいろんなケースがありますが、こう書くと読みやすい、直しやすいみないな話から、ベタ書きされるている値の変数化とか、いろんな話が入ってます。

Example

改行をたくさん含む文字列のheardoc化
要素の多いarrayの複数行化
より分かりやすい書き方のSuggestion
こんなコメントがあるとわかりやすい
UTCでしか設定できないCronの設定に対してJSTの時間をコメントで入れておくと、脳内変換がいらないとか
既存変数からとれる値（用途も同じ）ものはベタ書きしない

【3】設定／ロジックの不足（Bug含む）

SRE チームはロジックの実装よりも、例えば Terraform や KubernetesManifest などの「設定」が多いので、設定すべき値が足りていない、または間違っているようなケースに対するコメントが多いです。

特に環境間でコピーした際の修正漏れとか、その設定内容では期待する挙動にならない（Bug）などそういうコメントになります。

Example

Staging に接続を許可する IP のリストを設定する際に、既存のIPリストと比較するといくつか足りていない
Development から Staging に設定をコピーしている場合に、Development の設定値がそのまま残っている
L7 のルーティング設定変更をしているが、その設定では期待している挙動にならない
Kubernetes Manifest の設定項目が間違っている

【4】不要な設定／ロジック

その設定がなくても同じ挙動になる場合、あっても大きな害は無いのですが基本的には削除しておきたいと思う派です。将来、別のエンジニアが見たときに「なんでこれが設定されてるのだろう？もしかして必要無いのでは？」と時間を溶かしてしまわないようにするためです。

不要な設定を入れるのはとても簡単ですが、あとから背景を知らない人がそれを削除するのは調査やテスト含めて結構大変だったりします。

Example

どこからも参照されていない設定が入ってる
Development のみで必要だった設定が Staging にも入っている
Shell 内の一部ロジックが特に何もしていなそうなので不要

【5】 PRのスコープ

これは、PR そのものの確認や、PR の分割に対するコメントだったり、PR と開発のライフサイクルの整合性に関するコメントだったりします。

Example

全体の設計方針の確認と、その中でのこの PR の位置付け（スコープ）はどこになるのかを確認するコメント
リリースは先で PR をマージできないけど、試行錯誤を繰り返しそうな場合に、PR の組み方の変更を提案
マージしないと、dev&stg 確認できない場合は、prod の PR は分けてほしい
ディレクトリ構造を大きく変更する場合に、今後レビュワーが差分を見てレビューしやすいようなPRの段階的な組み方の提案

【6】原因の特定

「なぜか○○が動かなかったので、△△の対応しています」的なやつ、緊急の障害対応ではいいかもですが、できれば原因を明らかにして、最善の修正を加えたいなぁと思うのです。

Example

「なぜか○○が動かない」の原因を特定して、可能な限り Official Document や公式リポジトリの Issue でその事象についての説明がある部分を添えて伝える。
「なぜか○○が動かない」の原因特定まではしないにしても、原因となりそうな事象を伝える

【7】一貫性

個人的にはこれは結構重視したいポイントだったりします。

確かにそのコード（設定）は期待どおりに動くのだけど、特に意図なく他のところと書き方が全然違ったり実装方針が違ったりすると、後々そのコードにたどりつく人が「なぜ、ここだけ違う書き方なのだろう？」「どういう意図で変えているんだろう？」と時間を溶かしてしまうための原因になったりもするので、結構一貫性については重要なレビューポイントかなぁと思ったりしてます。

「Googleのソフトウェアエンジニアリング」の「8章スタイルガイドとルール」にも、

一貫性があれば、比較的少ない努力で比較的多くの仕事をやり遂げるエンジニアが増えるのだ

と書いてあり、コードレビュープロセスでも一貫性を重視するようか記述があります。

さいごに

実際に自分の直近のレビューコメントを集めることで、自分がどんなことに重点を置いてレビューしているのかが垣間見れた気がする。

結構いつも同じこと言ってるなぁｗって感想もあるので、こういったものが個人ごとの知識ではなく集合知としていい感じにブラッシュアップされていく仕組みなどを考えていけると面白そうかも。

Pull Requestのレビューで自分がどんなコメントしてるか見てみた

初めに

ここ1ヶ月くらいでどんなコメントしているか？

1. GitHub Explorer を使う

2. Script を書く

どんなコメントを最近していたか？

【1】 PR の Description の充実化・レビュワーの負荷軽減

【2】 可読性・保守性

【3】 設定／ロジックの不足（Bug含む）

【4】 不要な設定／ロジック

【5】 PRのスコープ

【6】 原因の特定

【7】 一貫性

さいごに

【2】可読性・保守性

【3】設定／ロジックの不足（Bug含む）

【4】不要な設定／ロジック

【6】原因の特定

【7】一貫性