コーディング規約チェックを日常に〜無理なくチーム全員で取り組める仕組みを作ろう

この記事はpepabo Advent Calendar 2016の4日目です。 3日目は@r_takaishiさんの「IIJmioのクーポン残量をAWS LambdaとMackerelでプロットしてみよう」でした。

コードレビューでよくある風景

突然ですが、コードレビューを行っていると、このようなコメントを一度はする・されると思います。

インデントやスペースの抜け漏れといった、細かいコーディングスタイルに関する指摘ですね。 人力でフォーマットの指摘をするのは見逃しが発生しやすく、細かい部分を確認するコストがかかります。小言のようなコメントになってしまいがちで、開発者・レビュアー双方に負担が増えてしまいます。

このような作業は機械に任せましょう。RuboCopやPHP-CS-Fixer、ESLintといったコーディング規約チェックツール(Lintツール)が役立ちます。 Lintツールを導入することでコーディング規約の実践にかかるコストを減らし、可読性・保守性の向上に貢献したり、より本質的な部分のレビューに時間を使えたりできることは、多くの方が実感していることでしょう。

既存プロジェクトに導入する際の課題

しかし、既存のプロジェクトにLintツールを導入するには課題があります。たとえば、

• 尋常じゃない量の違反が見つかってどうしたらいいかわからない😱
  • 一度に自動修正すると差分が多すぎて、きちんと動くのか検証が困難とか……
• 導入したはいいものの、開発サイクルの一部としてチームに定着できていない😇
  • 忘れる、面倒くさい、よくわからない→結局自分しかやってないとか……

Lintツールを導入すること自体が目的ではありません。Lintツールの助けを得て、継続的に、将来にわたって読みやすいコードを作っていくことが目的です。 せっかく導入しても開発サイクルの一部として定着しなければ意味がありません。

つまり、無理なくチーム全員で取り組めるような仕組みを作り、段階的に導入していくことが必要になるでしょう。

今回Lintツールを導入したチームも、上の課題に当てはまっていました。

• 既存のコードが大量にあり、一度に自動修正すると差分が多すぎる
• リポジトリにLintツール自体は入っていたが、活用されなかった悲しい過去が……😇

無理なくチーム全員で取り組めるような仕組み

では、「無理なくチーム全員で取り組めるような仕組み」とはなんでしょうか？ 様々あるかと思いますが、今回は以下の3つの対策を行うことで課題に対処しました。

1. Pull Request内で追加・変更したファイルのみを対象にチェックをかけ、段階的かつ確実に直していく
2. 言語をまたがってチェック・修正を一発でできるスクリプトを用意しておく
3. CI中に実行することで目に見えてわかる結果を出し、強制力をもって開発サイクルに載せる

では、ひとつずつ紹介していきます。サンプルリポジトリは以下にあります。

• shimoju/step-by-step-linting
• 追加分のコード: shimoju/step-by-step-linting/pull/1/files

Pull Request内で追加・変更したファイルのみを対象にチェックをかけ、段階的かつ確実に直していく

全ファイルを対象にチェックをかけると大量の違反が見つかり、自動修正しても差分が多すぎて正しく動くかの検証が困難になります。 そこで、新しく追加するコードに対してのみチェックをかけるようにします。

ブランチで追加・変更したファイルはgit diffのオプションを使って取得することができます。

git diff --name-only --diff-filter=ACM origin/master...HEAD

• トリプルドット(...)を使ってマージベース(ブランチを切った時点のmaster)と比較します
  • git diffコマンドで比較する時のダブルドット(..)とトリプルドット(…)の違いとは？ | Yakst
• --diff-filter=ACMで追加・変更したファイルのみ抽出します
  • ステータスはACDMRTUXB*がありますが、リネーム(移動)でチェックが走るのはどうかなということでACMとしてみました

対象ファイルを限定することで、新しく追加するコード、および差分が出たファイルは確実にコーディング規約が守られた状態になります。 少しずつではありますが、普段行う開発のついでに、無理なく自動的にコードがきれいになっていく世界を作ることができます。

言語をまたがってチェック・修正を一発でできるスクリプトを用意しておく

言語ごとにLintツールは異なるため、(共通点は多いものの)各ツールやコマンド体系を覚える必要があります。 たとえばフロントエンドのコーディングであれば、PHPテンプレート・CSS・JavaScriptファイルを同時に編集する場合もあるでしょう。このときにLintツールをそれぞれで実行する必要があるのは面倒です。

そこで以下のような一括チェック用のシェルスクリプトを用意しました。

#!/bin/bash

function diff-filter-ext() {
  git diff --name-only --diff-filter=ACM origin/master...HEAD | grep "\.$1$"
}

lint_langs=()
options=()
for arg in "$@"
do
  case "$arg" in
    # --で始まっていればオプションとみなし、それ以外はlintする言語指定とみなす
    --* ) options+=( "$arg" ) ;;
    * ) lint_langs+=( "$arg" ) ;;
  esac
done

# 言語指定がなければ全言語に対してlintをかける
if [ ${#lint_langs[@]} = 0 ]; then
  lint_langs=(php js)
fi

status=0
for lang in ${lint_langs[@]}
do
  diff-filter-ext $lang | xargs -t -n1 -I'FILE' bin/lint-$lang FILE ${options[@]}
  if [ $? != 0 ]; then status=1; fi
done

if [ $status != 0 ]; then exit 1; fi

さらに各ツールのインタフェースを合わせるためのラッパースクリプトを用意します。

これでbin/check-coding-ruleを実行すればすべてのLintツールでチェックが行われます。自動修正は--fixオプションで行なえます。 スクリプトはもうちょっといけてる感じにできると思いますが、ツールを意識することなく、コマンド一発でチェック・修正を行えるようにできました。

CI中に実行することで目に見えてわかる結果を出し、強制力をもって開発サイクルに載せる

最後に、コーディング規約チェックをCIで実行し、規約違反があった場合はCIを失敗させるようにします。目に見えてわかるようにすることで、規約チェックがあることを意識でき、自然とチェック・修正を行えるようになります。

実際の業務ではDroneを使っていますが、Travis CIでは以下のように設定できます。

language: php
php:
  - '7.0'
before_script:
  - nvm install 6.9 && nvm alias default 6.9
  - npm install
  - composer install
script:
  # コーディング規約チェックチェックスクリプトを実行する
  - bin/check-coding-rule
  - phpunit

規約違反があるとこのようにCIが失敗します。大抵のLintツールではどこがどのエラーなのか出力するようにできるので、設定しておくととても親切になります。

強制力をもつことになるため、突然導入されて戸惑わないよう、チームメンバーへ事前共有しておくのがよいでしょう。 ペパボではデザイナーがコーディングまで行うため、事前にデザイナーさんも交えて話を聞き、現状Lintツールが活用できていない要因と、このような対策をとろうと思いますということを話しました。 READMEやWikiに、よくあるエラーとその修正方法をまとめておくことも重要でしょう。

ESLintの例

今回はESLintを新規導入したため、そのTipsもいくつか載せておきたいと思います。

• すべてのルールのON/OFFが可能なため、コードの現状に合わせて設定することができる
• ESLintのルール一覧
  • "eslint:recommended"ではチェックマークのついたルールが有効になる
  • レンチマークは--fixオプションによる自動修正が行えるもの
• Stylistic Issues(,の後のスペースやインデントなど)はロジックに関わらない上に自動修正できるものが多いため、多めに入れておく
  • レビューのときに「細かいところですが…」と言うようなやつを大幅に減らせます
• 既存のコードのエラーを(雑でいいので)集計し、厳しそうなものをwarningに落とす
  • no-undef(宣言されていない変数の使用禁止)が一番多く、Pull Requestのついでに修正するには厳しそうなためwarningに落としました
• 今後も調整になるかと思いますが、.eslintrcはこのようにしました

まとめ

コーディング規約チェックを「無理なくチーム全員で取り組めるような仕組みを作る」ことを目指しました。 そのために、以下の対策を行いました。

1. Pull Request内で追加・変更したファイルのみを対象にチェックをかけ、段階的かつ確実に直していく
2. 言語をまたがってチェック・修正を一発でできるスクリプトを用意しておく
3. CI中に実行することで目に見えてわかる結果を出し、強制力をもって開発サイクルに載せる

気になる結果ですが、ESLintの追加はマージできていないため、残念ながら結果はこれからとなります…。 しかし、PHP-CS-Fixerは既に導入されており、非常によいサイクルが回っています。日常的にコーディング規約チェックを行うことができ、既に当たり前のようになっています。 JavaScriptのチェックも同じコマンドで同時にできるようにしたため、同様に良い結果となることを期待しています。

みなさんも、無理なく自動的にソースがきれいになっていく世界、作ってみませんか？

おまけ

記事の内容からは外れるため特に書いていませんでしたが、このタスクは新卒研修のOJTで配属されたチームで行ったものです。 PHP-CS-Fixerの最初の導入は同期エンジニアが行い、ESLintの追加・チェックスクリプトの多言語対応を私が行いました。

ペパボの新卒エンジニア研修については、GMOペパボの新卒エンジニア研修の様子 & テキストを公開します - ペパボテックブログを見ていただけると様子がわかりますので、ぜひご覧いただければと思います。