Deno で大きなプルリクエストを未然に防ぐための CLI を作った

この記事は GiXo アドベントカレンダーの21日目の記事です。
昨日は「マイグル　チーム構成の紹介」でした。

Technology Div. の堀越です。今年の Advent Calendar も本記事を含め残すところあと5記事となり終わりが見えてきました。私の担当記事はこれで最後のため、気分は年末です。

本記事ではフロントエンドチームのプロダクト開発におけるある課題を解決するために考えたことと、それに伴い実装してみた CLI を紹介いたします。そのある課題とはタイトルにもある通り「大きなプルリクエスト」です。「大きなプルリクエスト」が一様に悪い訳ではないかもしれませんが、我々の現場におけるレビューのつらさ・難しさは他にも原因はあれど「プルリクエストの大きさ」に比例しているため、「大きなプルリクエスト」と書きました。

なお「大きなプルリクエスト」のサイズ感はチームやプロジェクト、開発する対象によって異なると思いますが、本記事では「変更行数の合計が1,000行を超えるプルリクエスト」は「大きなプルリクエスト」という感覚で書いています。

1 背景
- 1.1 プルリクエストとコードレビュー
2 （余談）トランクベース開発
3 Deno で CLI を実装する
4 おわりに

背景

弊社ではソースコードのバージョン管理ツールに GitHub を利用しています。GitHub を使っている個人・企業は多いと思いますが、みなさんは適切な粒度・サイズのコミットやプルリクエストを作れていますか？私は微妙というのが正直なところです。しかしチームで開発を進めている以上、コミットやプルリクエストをおざなりにして良い開発体験を継続することはできません。

弊チームは GitHub Flow に則って開発を進めています。新しい機能追加やリファクタリングをする際はメインブランチから開発用のブランチを切り出し、目的とする開発が完了したらプルリクエストを出します。コードレビューが完了して問題なく Approve されたらすぐにメインブランチへマージ → 検証環境デプロイ → 本番リリースという流れです。

プルリクエストとコードレビュー

個人的な意見ですが、前述の開発の流れにおいて最も重要な要素はプルリクエストです。自分の書いたコードをプルリクエストによってメンバーにレビューしてもらい、問題なければマージしてリリースする、という流れはプロダクト開発の根幹となる作業です。プルリクエストにおけるレビューが適切になされていないと、開発者自身では気づけなかった実装のバグや動作確認の漏れがプロダクションコードに混入するリスクが高まります。また実装の詳細を開発者自身しか把握できていない状態だと、その後の機能追加や改修が属人化してしまうかもしれません。

上記のように思いつつも常にやりたいこと・やるべきことを抱えて走り続けていると、「何のためにプルリクエストという工程を挟んでいるのか？」に対する意識が薄まっていることがしばしばです。目の前のバグを取り除くことや機能追加を急ぐあまり、様々な実装が入り混じったコミットと巨大なプルリクエストを生み出してチームや自分自身を疲弊させてしまうこともあります。

適切な粒度・サイズでコミットとプルリクエストを作成すべきだと頭ではわかっていても、気づくと目の前のコードの実装が気になってリファクタリングを始めてしまったり、ついでの機能改修に手をつけてしまったり。。ブランチ名と実際の作業内容の乖離に気づいても時既に遅し。。といったことが日常茶飯事では困りますが、プロダクト開発現場あるあるのひとつだと思っています。（自戒を込めた個人の意見です）

前置きが長くなりましたが、頭ではわかっていても同じ失敗を繰り返してしまう、そんなときは作業のプロセスや仕組みを見直すしかないのでは？ということで今回実装したものが「大きなプルリクエストを未然に防ぐための CLI」です。

（余談）トランクベース開発

前の章で「GitHub Flow で最も重要な要素はプルリクエスト」と書きましたが、果たして本当にコードレビューを介することでバグや適切でない実装を防ぐことはできるのか？という意見や考えもときどき目にします。これについては自分も思い当たる節があり、例えば忙しく仕事に追われている状況だとコードレビューや動作確認がおざなりになって、ざっと見ただけで LGTM!! ということがないとは言えません。すみません。

GitHub で変更の差分として表示されている箇所だけ見て問題がなくても、多くの場合その変更箇所は別のモジュールに依存していたり依存されていたりします。差分を見るだけで依存モジュールへの影響を全て見抜くのはなかなか難しいですね。

そうした状況の打開策のひとつとしてトランクベース開発があると思っています。Google Cloud ドキュメントのトランクベース開発の解説が端的にまとまっておりわかりやすいのですが、個人的なポイントは下記の部分です。

自分の作業を小さなバッチに分割し、その作業を 1 日に少なくとも 1 回（場合によっては数回）トランクにマージします。このアプローチの主な違いはスコープです。通常、機能ブランチには複数のデベロッパーが関与し、作業が終わるまでに数日から数週間かかります。対照的に、トランクベース開発のブランチは数時間以内で終わり、多くのデベロッパーが個々の変更を頻繁にトランクにマージします。
出典: Google Cloud: DevOps 技術: トランクベース開発

小さな改修であればコードベース全体への影響は把握しやすく、レビュアーの負担も小さくなります。リリースされている実装と追加開発された実装の乖離をミニマムにできるため、動作確認の頻度は上がりますがリリース後の予期せぬバグに対する不安は小さくなり、仮にバグがあっても安定版へのロールバックも比較的用意です。レビューやリリースに際する作業は増えるため自動化や仕組み化による作業負担の軽減はマストですが、それをクリアできれば恩恵が大きそうかつテンポよく開発できそうなスタイルだなと思いました。

なぜこの余談を挟んだのかというと、自分が今回実装した CLI は開発体制をトランクベース開発に近づけるための取り組みのひとつともとれるなあと思ったためです。

Deno で CLI を実装する

Deno を選んだ理由は自分の主な開発言語が TypeScript だからです。また、インストールさえすればゼロコンフィグで使えて、CLI ツールの配布とインストールを簡単に行うためのコマンドが用意されているのも便利です。ロゴがかわいいのもポイント高いですね。

解決したい課題とその原因

今回解決したかった課題は「大きなプルリクエスト」です。これを未然に防ぐために必要なことはなんでしょうか？

自分の場合ですが、「大きなプルリクエスト」は「複数の目的が入り混じったコミット」によって生まれることが多いです。単一の機能や目的に閉じた実装であればプルリクエストは（比較的）巨大化しにくいですし、レビュアーの負担も小さくできます。

どんなレビューがつらいかって仮にひとつあげるとすれば、「目的が異なる変更の差分をまとめて渡されて、それをファイルごとに紐解きつつ動作確認しなければならないケース」でしょうか。つらいレビューの定義もしたいところですが今回は割愛します。

それではなぜ「複数の目的が入り混じったコミット」が生まれるのでしょうか？これは前の章でも書きましたが、

目の前のコードの実装が気になってリファクタリングを始める
ついでの機能改修に手をつける（そんなに大きくないからと言い訳しながら）

自分の場合はこの2つが二大巨頭です。この2つを未然に防げれば課題解決に近づける気がします。それでは続いてどうすればこの2つを未然に防げるかを考えていきます。

課題解決の方法

前述の二大巨頭の発生を防ぐ方法としては、例えば「プルリクエストには目的を2つ以上含めてはならない。2つ以上含まれていたらレビュー以前の問題としてリジェクトする」というチーム内のルールを設けて運用する（メンバーの意識に訴えかける）方法があります。

しかしルールを設けるだけで人が変われるなら私は今頃筋骨隆々な肉体を手にしているはずです。開発チームが様々なタスクに追われて目の前の作業を早く終わらせることに集中し始めると、暗黙的な了解で上記ルールが破られる日は遠からず必ずやってくる、と思っています。少なくとも自分の場合は意識や心がけに頼った方法では現実的な解決策になりません。残念ですが。

ということで意識改革に頼らず、前述の二大巨頭を防ぐために自分の考えた方法は「実装の差分情報を認知しやすい形で動的かつ継続的にフィードバックする仕組み」です。いきなり具体的になったためもう少し抽象的にいうと「複数の目的が入り混じった実装になりつつあることを開発者が認知するための仕組み」です。これを CLI として実装しました。

成果物

前振りは長いですが大層なものを作ったわけではなく、git の CLI の出力を今回の目的に合わせてちょいちょいと加工・出力・通知する処理を追加したものが今回の実装です。

実際の動作を動画で貼っておきます。（VSCode 右端のターミナルで今回実装した CLI が実行されています。/tmp/tmp.ts, /tmp/tmp2.ts というファイルを追加した際にその変更差分が表示に反映されています）

git の CLI を使っている方なら動画を見てお気づきかもしれませんが、利用しているコマンドは git diff --stat コマンドです。このコマンドだけでも情報量としては十分ですが、今回は

認知負荷を低くできる
自分たちのルールに従ってフィードバックの内容を動的に変えられる
適宜フィードバックする

といった要素が欲しかったため手を加えました。ちなみに git diff --stat そのものの出力は下記です。

今回実装した CLI では変更行数に対して閾値を設けて、その閾値ごとにアイコンを用意しています。プルリクエスト・コミットを作る目安をアイコンにすることで直感的に今の差分状況を把握することを期待しています。

「大きなプルリクエストと複数の目的が入り混じったコミット」を未然に防ぐには、実装中に差分の状況を随時把握する必要があります。これは自分の場合ですが、「複数の目的が入り混じったコミット」は意識的に作っているのではなく、「無意識に当初目的から外れた実装に手をつけている」ことから生まれます。よってこの CLI を使う場合、実装中は常にフィードバックを受け取れるように Terminal ウィンドウをディスプレイのどこかに配置して CLI を走らせておきます。任意の閾値を超えたら Slack のチャンネルにメンションを投げる機能もつけているため、他人の目という圧力を加えることも可能です。

デフォルトでは diff（ソースコードの変更行数）の大きさをアイコンに変換する閾値を [50, 100, 200, 400] としています（引数で閾値を変えることも可能）。アイコンは下記の5種類です。
1. ?（0〜50行の diff）
2. ? （50〜99行の diff）
3. ? （100〜199行の diff）
4. ? （200〜399行の diff）
5. ? （400行〜の diff）

diff の大きさで適切なコミットの粒度やプルリクエストを画一的に測ることは難しいため一概には言えないですが、100行を超えたらもうコミット（プルリクエスト）を検討してもよいだろうということで ?（成熟）です。例えば「? が2羽誕生したらプルリクエスト」のように基準を決めておくことで、小さなプルリクエストを積み重ねる意識づけを自分に期待しています。

アイコンはファイルごとの変更行数だけでなく、メインブランチとの差分に対しても出力しています。プルリクエストについてはメインブランチとの差分の方が重要なので、個々のファイルの状況がどうであれ全体として? になる前にプルリクエストを出すことが基本方針となるよう閾値を設定します。

400行を超える行数の変更を避ける

ちなみにデフォルトの閾値の最大値として設定した400行という数字には一応意味がありまして、ちょうどよい閾値を検討するために調査する中で見つけた SmartBear Software の Best Practices for Code Review^[1] を参考にしました。

この記事の1項目で SmartBear がシスコシステムズプログラミングチームについて調査を行なった結果が紹介されており、下記のように書かれています。

1. Review fewer than 400 lines of code at a time

A SmartBear study of a Cisco Systems programming team revealed that developers should review no more than 200 to 400 lines of code (LOC) at a time. The brain can only effectively process so much information at a time; beyond 400 LOC, the ability to find defects diminishes.

In practice, a review of 200-400 LOC over 60 to 90 minutes should yield 70-90% defect discovery. So, if 10 defects existed in the code, a properly conducted review would find between seven and nine of them.
出典: SmartBear Software: Best Practices for Code Review

Code Review Best Practices — 出典: SmartBear Software: Best Practices for Code Review

意訳すると「400行を超える行数の変更を一度にレビューしようとすると欠陥を見つける検知能力が下がる」というデータが出ているそうです。この400行という数字は個人の感覚ともなんとなく一致しており、400行未満のプルリクエストなら見通しもよく心理的負担があまり大きくありませんが、500行を超えているともうそのようには思えません。

この辺は実際に運用を継続してみないとなんとも言えませんが、この CLI を開発中のリポジトリで走らせることで課題の解決に近づくことを期待しています。

実装について

Deno による CLI の実装についても軽く触れておきます。全体を載せるといささか長い & 個人開発でとりあえず動くところまで作っただけで説明もないため、メイン部分のみ抜粋しています。

import { ansi } from "https://deno.land/x/cliffy@v0.20.1/ansi/mod.ts";
import { ensureFile } from "https://deno.land/std@0.118.0/fs/mod.ts";
import { parse } from "https://deno.land/x/std@0.118.0/flags/mod.ts";

import { makeDiffMessage } from "./makeDiffMessage.ts";
import { notifyDiffToSlack } from "./notifyDiffToSlack.ts";
import { parseGitDiff } from "./parseGitDiff.ts";
import { prevLinesPath } from "./constants.ts";

const args = parse(Deno.args);

// 実行間隔（s）
const intervalSeconds = Number(args.intervalSeconds) || 2;

// diff に対応するアイコンの閾値
let thresholds = [50, 100, 200, 400];
if (args.thresholds) {
  const splitThresholds = args.thresholds.split(",");
  if (!Array.isArray(splitThresholds))
    throw new Error("Invalid thresholds value.");

  thresholds = splitThresholds
    .filter((th: unknown): th is string => !!th)
    .map((th: string) => Number(th))
    .sort((a, b) => a - b);
}

// diff を取得するファイルの拡張子
let includeExtensions: string[] = [];
if (args.includeExtensions) {
  const splitExtensions = args.includeExtensions.split(",");
  if (!Array.isArray(splitExtensions))
    throw new Error("Invalid includeExtensions value.");

  includeExtensions = splitExtensions
    .filter((ext: unknown): ext is string => !!ext)
    .map((ext: string) => (ext.startsWith(".") ? ext : `.${ext}`));
}

let count = 0;
const run = async () => {
  const diffFromPrevCommit = await parseGitDiff({
    includeExtensions,
  });
  const diffFromPrevCommitResult = await makeDiffMessage({
    diffCountPerFiles: diffFromPrevCommit.diffCountPerFiles,
    filesChangedInfo: diffFromPrevCommit.filesChangedInfo,
    maxDigit: diffFromPrevCommit.maxDigit,
    thresholds,
  });

  const diffFromMainBranch = await parseGitDiff({
    includeExtensions,
    branchName: "origin/main",
  });
  const diffFromMainBranchResult = await makeDiffMessage({
    diffCountPerFiles: diffFromMainBranch.diffCountPerFiles,
    filesChangedInfo: diffFromMainBranch.filesChangedInfo,
    maxDigit: diffFromMainBranch.maxDigit,
    thresholds,
  });

  const fileDiffArr = diffFromPrevCommitResult.fileDiffArr.concat(
    diffFromMainBranchResult.fileDiffArr
  );
  const fileSumDiffArr = diffFromPrevCommitResult.fileSumDiffArr.concat(
    diffFromMainBranchResult.fileSumDiffArr
  );
  const message = `
#===========================#
# Diff from Previous Commit #
#===========================#
${diffFromPrevCommitResult.message}

#===========================#
# Diff from Main Branch     #
#===========================#
${diffFromMainBranchResult.message}
`;

  if (count > 0) {
    const prevLines = Number(await Deno.readTextFile(prevLinesPath)) || 0;
    console.log("" + ansi.eraseLines(prevLines + 1) + ansi.cursorUp());
    console.log(message);
  } else {
    console.log(message);
  }

  await notifyDiffToSlack({
    fileDiffArr,
    fileSumDiffArr,
    message,
  });

  await ensureFile(prevLinesPath);
  await Deno.writeTextFile(prevLinesPath, String(message.split("\n").length));
  count++;
};

await run();
setInterval(async () => {
  await run();
}, intervalSeconds * 1000);

100

101

102

103

import { ansi } from "https://deno.land/x/cliffy@v0.20.1/ansi/mod.ts";

import { ensureFile } from "https://deno.land/std@0.118.0/fs/mod.ts";

import { parse } from "https://deno.land/x/std@0.118.0/flags/mod.ts";

import { makeDiffMessage } from "./makeDiffMessage.ts";

import { notifyDiffToSlack } from "./notifyDiffToSlack.ts";

import { parseGitDiff } from "./parseGitDiff.ts";

import { prevLinesPath } from "./constants.ts";

const args = parse(Deno.args);

// 実行間隔（s）

const intervalSeconds = Number(args.intervalSeconds) || 2;

// diff に対応するアイコンの閾値

let thresholds = [50, 100, 200, 400];

if (args.thresholds) {

const splitThresholds = args.thresholds.split(",");

if (!Array.isArray(splitThresholds))

throw new Error("Invalid thresholds value.");

thresholds = splitThresholds

.filter((th: unknown): th is string => !!th)

.map((th: string) => Number(th))

.sort((a, b) => a - b);

}

// diff を取得するファイルの拡張子

let includeExtensions: string[] = [];

if (args.includeExtensions) {

const splitExtensions = args.includeExtensions.split(",");

if (!Array.isArray(splitExtensions))

throw new Error("Invalid includeExtensions value.");

includeExtensions = splitExtensions

.filter((ext: unknown): ext is string => !!ext)

.map((ext: string) => (ext.startsWith(".") ? ext : `.${ext}`));

}

let count = 0;

const run = async () => {

const diffFromPrevCommit = await parseGitDiff({

includeExtensions,

});

const diffFromPrevCommitResult = await makeDiffMessage({

diffCountPerFiles: diffFromPrevCommit.diffCountPerFiles,

filesChangedInfo: diffFromPrevCommit.filesChangedInfo,

maxDigit: diffFromPrevCommit.maxDigit,

thresholds,

});

const diffFromMainBranch = await parseGitDiff({

includeExtensions,

branchName: "origin/main",

});

const diffFromMainBranchResult = await makeDiffMessage({

diffCountPerFiles: diffFromMainBranch.diffCountPerFiles,

filesChangedInfo: diffFromMainBranch.filesChangedInfo,

maxDigit: diffFromMainBranch.maxDigit,

thresholds,

});

const fileDiffArr = diffFromPrevCommitResult.fileDiffArr.concat(

diffFromMainBranchResult.fileDiffArr

);

const fileSumDiffArr = diffFromPrevCommitResult.fileSumDiffArr.concat(

diffFromMainBranchResult.fileSumDiffArr

);

const message = `

#===========================#

# Diff from Previous Commit #

#===========================#

${diffFromPrevCommitResult.message}

#===========================#

# Diff from Main Branch #

#===========================#

${diffFromMainBranchResult.message}

if (count > 0) {

const prevLines = Number(await Deno.readTextFile(prevLinesPath)) || 0;

console.log("" + ansi.eraseLines(prevLines + 1) + ansi.cursorUp());

console.log(message);

} else {

console.log(message);

}

await notifyDiffToSlack({

fileDiffArr,

fileSumDiffArr,

message,

});

await ensureFile(prevLinesPath);

await Deno.writeTextFile(prevLinesPath, String(message.split("\n").length));

count++;

};

await run();

setInterval(async () => {

await run();

}, intervalSeconds * 1000);

これだけ見せられても何のこっちゃですが、ざっくりとした処理の流れは下記の通りです。

引数を取得（diff 出力の実行間隔秒数、閾値、diff を取得するファイル拡張子）
git diff –stat を実行して出力をパース
パースして取得した値を加工して出力メッセージを作成
メッセージを出力
Slack に通知（あらかじめ設定された条件を満たしていたら）

他にも処理が記載されていますが、出力をキャッシュして前の出力との差分検知をしたり履歴を保存したり、といったことを裏で行なっています。

個人的なこだわりポイントは diff を出力するたびに新たな出力が流れるのではなく、元の出力が上書きされるようにしたところです。プログレスバーと同じですね。Docker image をビルドする際に複数のプログレスバーが流れますが、あんな感じです。単一行の上書きならキャリッジリターンを使えば手軽にできるみたいですが、複数行の動的な上書きはそれではうまくできなかったため、ソースコードにある通り Cliffy の ansi を使ってカーソルの制御と出力の上書きを行っています。eraseLines によって出力がチラつかないか心配だったのですが、そんなこともなく滑らかに上書きしてくれます。（実装部分は下記）

    const prevLines = Number(await Deno.readTextFile(prevLinesPath)) || 0;
    console.log("" + ansi.eraseLines(prevLines + 1) + ansi.cursorUp());
    console.log(message);

const prevLines = Number(await Deno.readTextFile(prevLinesPath)) || 0;

console.log("" + ansi.eraseLines(prevLines + 1) + ansi.cursorUp());

console.log(message);

なお、この CLI によってプルリクエストやコミットのサイズがこう変わった！というデータはまだ取れていません。この CLI が良い感じに活躍したら before / after のデータをとって比較してみたいですね。

おわりに

本文でも少し触れましたが、diff の大きさによって適切なプルリクエストのサイズを測れるわけではありません。diff を大きくしないようにと意識するあまり、加えておいた方が結果としてハッピーになるような変更をやめて（面倒だからと）別のプルリクエストを立てることもやめる・後回しにする、というのは本末転倒に思えます。その辺の塩梅はチームでコミュニケーションをとるのがよいでしょう。

大した変更ではないのに diff が大きいからとプルリクエストを分けることを面倒に思うケースはあるでしょうが、それでも「大きなプルリクエストと複数の目的が入り混じったコミット」が生まれるよりはマシな気がします。諸事情で妥協することがあったとしても、 diff を意識してコミットやプルリクエストを小さくまとめるように努めるメリットは大きいと思った次第です。

プルリクエストをいくつも作るのが面倒ならプルリクエストを作る仕組みそのものも自動化するなり効率化すればよいわけで、今もテンプレートは使っていますが、これを機に色々見直したいと思いました。

本記事を通して GiXo でのプロダクト開発に少しでも興味をもっていただけたら幸いです。詳細を知りたい方がいましたらぜひお問い合わせください。採用についてはカジュアル面談も実施しています。採用情報はこちら。

明日は Design & Science Div. の遠藤より、「キャンペーンを企画するとき、まず考えたい目的と手段の話」を公開予定です。

[1] 出典: SmartBear Software: Best Practices for Code Review

Go Horikoshi
Data-Informed 事業本部 / Technology Div. 所属
フロントエンドエンジニア。React, TypeScript を中心とした Web フロントエンドについて発信していきます。最近の興味はサーバーサイド JavaScript（エッジや Service Worker）とコード生成による開発の効率化です。

Deno で大きなプルリクエストを未然に防ぐための CLI を作った

背景

プルリクエストとコードレビュー

（余談）トランクベース開発

Deno で CLI を実装する

解決したい課題とその原因

課題解決の方法

成果物

400行を超える行数の変更を避ける

実装について

おわりに

SERVICE

DIコンサルティング

DIプラットフォーム

TAG BOX