この記事はpepabo Advent Calendar 2016 の4日目です。@r_takaishi さんの「IIJmioのクーポン残量をAWS LambdaとMackerelでプロットしてみよう 」でした。
コードレビューでよくある風景 突然ですが、コードレビューを行っていると、このようなコメントを一度はする・されると思います。
インデントやスペースの抜け漏れといった、細かいコーディングスタイルに関する指摘ですね。
このような作業は機械に任せましょう。RuboCop やPHP-CS-Fixer 、ESLint といったコーディング規約チェックツール(Lintツール)が役立ちます。
既存プロジェクトに導入する際の課題 しかし、既存のプロジェクトにLintツールを導入するには課題があります。たとえば、
尋常じゃない量の違反が見つかってどうしたらいいかわからない😱
一度に自動修正すると差分が多すぎて、きちんと動くのか検証が困難とか……
導入したはいいものの、開発サイクルの一部としてチームに定着できていない😇
忘れる、面倒くさい、よくわからない→結局自分しかやってないとか……
Lintツールを導入すること自体が目的ではありません。Lintツールの助けを得て、継続的に、将来にわたって読みやすいコードを作っていくことが目的です。
つまり、無理なくチーム全員で取り組めるような仕組みを作り、段階的に導入 していくことが必要になるでしょう。
今回Lintツールを導入したチームも、上の課題に当てはまっていました。
既存のコードが大量にあり、一度に自動修正すると差分が多すぎる
リポジトリにLintツール自体は入っていたが、活用されなかった悲しい過去が……😇
無理なくチーム全員で取り組めるような仕組み では、「無理なくチーム全員で取り組めるような仕組み」とはなんでしょうか？
Pull Request内で追加・変更したファイルのみを対象にチェックをかけ、段階的かつ確実に直していく
言語をまたがってチェック・修正を一発でできるスクリプトを用意しておく
CI中に実行することで目に見えてわかる結果を出し、強制力をもって開発サイクルに載せる
では、ひとつずつ紹介していきます。サンプルリポジトリは以下にあります。
Pull Request内で追加・変更したファイルのみを対象にチェックをかけ、段階的かつ確実に直していく 全ファイルを対象にチェックをかけると大量の違反が見つかり、自動修正しても差分が多すぎて正しく動くかの検証が困難になります。
ブランチで追加・変更したファイルはgit diffのオプションを使って取得することができます。
1 git diff --name-only --diff-filter=ACM origin/master...HEAD
トリプルドット(...)を使ってマージベース(ブランチを切った時点のmaster)と比較します
--diff-filter=ACMで追加・変更したファイルのみ抽出します
ステータスはACDMRTUXB*ACMとしてみました
対象ファイルを限定することで、新しく追加するコード、および差分が出たファイルは確実にコーディング規約が守られた状態になります。
言語をまたがってチェック・修正を一発でできるスクリプトを用意しておく 言語ごとにLintツールは異なるため、(共通点は多いものの)各ツールやコマンド体系を覚える必要があります。
そこで以下のような一括チェック用のシェルスクリプト を用意しました。
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 #!/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 --* ) options+=( "$arg " ) ;; * ) lint_langs+=( "$arg " ) ;; esac done 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では以下のように設定できます。
1 2 3 4 5 6 7 8 9 10 11 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ツールではどこがどのエラーなのか出力するようにできるので、設定しておくととても親切になります。
強制力をもつことになるため、突然導入されて戸惑わないよう、チームメンバーへ事前共有しておくのがよいでしょう。
ESLintの例 今回はESLintを新規導入したため、そのTipsもいくつか載せておきたいと思います。
すべてのルールのON/OFFが可能なため、コードの現状に合わせて設定することができる
ESLintのルール一覧
"eslint:recommended"ではチェックマークのついたルールが有効になるレンチマークは--fixオプションによる自動修正が行えるもの
Stylistic Issues (,の後のスペースやインデントなど)はロジックに関わらない上に自動修正できるものが多いため、多めに入れておく
レビューのときに「細かいところですが…」と言うようなやつを大幅に減らせます
既存のコードのエラーを(雑でいいので)集計し、厳しそうなものをwarningに落とす
no-undef (宣言されていない変数の使用禁止)が一番多く、Pull Requestのついでに修正するには厳しそうなためwarningに落としました
今後も調整になるかと思いますが、.eslintrcはこのようにしました
まとめ コーディング規約チェックを「無理なくチーム全員で取り組めるような仕組みを作る」ことを目指しました。
Pull Request内で追加・変更したファイルのみを対象にチェックをかけ、段階的かつ確実に直していく
言語をまたがってチェック・修正を一発でできるスクリプトを用意しておく
CI中に実行することで目に見えてわかる結果を出し、強制力をもって開発サイクルに載せる
気になる結果ですが、ESLintの追加はマージできていないため、残念ながら結果はこれからとなります…。
みなさんも、無理なく自動的にソースがきれいになっていく世界、作ってみませんか？
おまけ 記事の内容からは外れるため特に書いていませんでしたが、このタスクは新卒研修のOJTで配属されたチームで行ったものです。
ペパボの新卒エンジニア研修については、GMOペパボの新卒エンジニア研修の様子 & テキストを公開します - ペパボテックブログ を見ていただけると様子がわかりますので、ぜひご覧いただければと思います。
