Sider Blog

コードレビュー自動化サービス「Sider」を運営するSideCI株式会社のコーポレートブログです。



SideCIはSiderにサービス名変更しました

f:id:sideci:20180612140239p:plain

こんにちは。SideCIの角です。 本日2018年6月13日より、SideCIの製品名をSider(サイダー)へ変更いたします。 背景として、SideCIの「CI」は「継続的な開発プロセスの改善」を意図したサービス名でした。 しかし、「CI」は「継続的なテストとデリバリ」のことであるという認識も一般的です。そのため、SideCIが継続的なテストとデリバリのための製品であると認識されてしまうことがありました。

そのため、SideCIはコードレビューを支援するサービスであることを明瞭にするために、サービス名をSiderに変更することに決定しました。 Siderは「Side + Review」の造語です。今後も皆様のSide(となり)でレビューを支援するサービスとして、提供、改善を続けて参ります。

【変更の概要】

【旧ドメイン、URLについて】 新しいドメイン(sider.review) にリダイレクトされます。 そのため、直ちに何かをしていただく必要はございませんが、これまでのURLをブックマークしていただいている方などは、お手数ですがご変更をお願いいたします。

なお、社名はSideCI株式会社から変更ありません。

何かご不明点がありましたらサポートまでご連絡下さい。 引き続き、ご支援のほど何卒お願い申し上げます。

RubyKaigi 2018 直前!チーフオーガナイザ・松田明さん特別インタビュー

目次

今回は、松田明さんにお話をうかがう機会をいただきました。 松田さんと言えば、RubyとRuby on Railsのコミッタであり、世界で一番活発に活動している地域Rubyコミュニティの一つである「Asakusa.rbの創設者」でもあり、さらに世界最大のRubyカンファレンスである「RubyKaigiのチーフオーガナイザ」で、加えてKaminariActionArgsなどを開発した「Gem作家」でもある、と活躍の幅が広くて紹介しきれません。

松田さんにインタビューした記事は、すでに世の中にいくつかあるのですが、今回はあまりお話しされているのを目にしたことがない「コードレビュー」や「レビュー自動化」を中心に、お聞きしていきたいと思っています。

f:id:sideci:20180514133429j:plain

CRubyでのコードレビュー

ー先日SideCIのBlogで、まつもとゆきひろさんのインタビューを記事にしたものがありましたが、その中で、まつもとさんにコードレビューについてお話を聞きました。松田さんから見たときに、補足やあるいは異論なんかはありますか?

この話は面白いですね。仕事でもオープンソースでも「コミットしたコードを自分が将来的にメンテナンスすることになるけど、それをできるのか」というのが基準になっているとのことですが、これは、すごくちゃんと言語化されてて凄いなと思いました。その通りですよね。「コードを引き受ける決心」という短い表現もありましたが、なるほど、と。言語化が上手い人だなと改めて思いました。

ここで、まつもとさんがレビューについて話しているのは、これは恐らく彼が最近やっているmrubyにおける話ですね。これはいわゆるCRubyとはちょっと違います。

CRubyプロジェクトにおいては、そもそもコードレビューという概念がなくて、コミッターは書いたコードをSubversionのtrunkに直接コミットしています。Pull Requestを出して事前にCIを回して、みたいな感じでコミットするまでの間に誰かの目を通す仕組みというかプロセス自体がないんですよ。

そうしてtrunkにどんどんコードがコミットされるわけですが、そこでRubyCIというものがあって、定期的にtrunkから最新のコードをチェックアウトして、Rubyがサポートしている環境でテストを実行して、結果を表示します。マトリクスがすごくいっぱいあって、見ている人はこれを見ていると。だいたいそんな感じで回ってます。

それから、CRubyで一番レビューに近い何かがあるとしたら、それはPB memoという、コミッターのnagachikaさんが毎日欠かさず更新されているブログですね。nagachikaさんは全てのコミットを監視しているので、何かが壊れたらここに書かれて、それで他の人も気付いて次の日に修正される、みたいな。

ーいきなりtrunkが壊れると聞くと、けっこう乱暴な話にも聞こえますが…

でもtrunkだから壊れててもいいんじゃないですかね。リリースまでにはだいたいちゃんと直るので。

ーそれに対してmrubyはGitHubでということですか。

そうですね。

CRubyに関してはこういった感じで、特に誰もレビューもしてないし、すごく頻繁に壊れているというのが、おそらく反省点としてあって、まつもとさんが次のRuby処理系を作るプロジェクトを始めた時に、GitHubでやってみようってことにしたんだと思うんですよね。

mrubyでは、直接masterにpushされるのではなく、Pull Requstを作ってCIを回してレビューをしてからマージされるといういまどきっぽいプロセスですね。これは、まつもとさん1人の手に負えるサイズでやっているというのもあると思うんですが、CRubyの今の開発体制に対する、まつもとさんなりの反省というか改善も入っているんじゃないかと思ってます。

コードスタイルと規約

CRubyのコードスタイル

ーコードのスタイルをどうしていくか、みたいな問題は、Rubyの開発ではどうなっているんでしょうか?

CRubyのコードに関しては、コードスタイルの統一みたいな問題は、さっきのまつもとさんのインタビューで言われてたとおり「元々のファイルの雰囲気を読んで、各自好きなスタイルでいい感じになるようにコミットしてね」という感じです。コーディング規約とかは特にありません。すると実際にどうなっているかというと、たとえばよく話題になるのは、1つのファイル内にタブとスペースが混在してたりします。適当なCのファイルを開くとすぐわかると思うんですが、インデントがぐちゃぐちゃに入り乱れていて、なんかリアス式海岸みたいになってます。

f:id:sideci:20180529145910j:plain リアス式海岸。Photo by Serkan Turk on Unsplash

ー私もcompile.cとかを読んだときに困りました。8タブ表示にするとなんとか読めるようになる。

f:id:sideci:20180529152147p:plain 標準的なタブ幅2の設定で表示したときのcompile.cの抜粋。whileよりもifが左に表示されるため、コードの構造と一致しておらず、理解を妨げる。

f:id:sideci:20180529152204p:plain タブ幅8で表示すると、while内のifの位置がコードの構造と一致するようになる。

僕の手元のエディターだと2タブなので、凄くキザギザになっちゃいますね。そこで、mrknさんが作ったRuby開発者必携のVimプラグインがあって、これを入れておくとそこらへんをうまいこと吸収してくれるようになってます。

https://github.com/mrkn/vim-cruby

まぁ、ソースコードを読み書きするために、こういう努力が必要になっちゃうわけです。

ーちなみに、コミッターの皆さんの間ではタブとスペースのどっちが優勢みたいな感じはあるんですか?

人数はスペース派の方が多いはずです。でもパッチモンスターの中田さんがどちらかというとタブ派なので手数が多いという。

笹田さんはタブですね。中田さんはどっちでもいいと言ってるんですが、最近はタブにしているようです。そしてスペース派の急先鋒は、僕が把握してる限りakrさんなんですかね。akrさんがコツコツと地道に、自分がいじった行はタブをスペースに書き換えてコミットするみたいな活動をやっておられたはずです。そしてまた別の人が書き換えた時にはスペースがタブに置き換えられたりする、タブスペース合戦みたいなことが繰り広げられていたりします。そこに3タブ派が入るとさらにカオス状態になるかもしれないですね。幸いCRubyには3タブ派はいないと思いますが。

このタブスペースの話が一例ですけど、コードスタイルをみんなで足並み揃えてなにかにしようというのはとにかく一切ないです。

その結果、GitHubのレポジトリを作ってプルリクエストを受け付けるようになってみると、コントリビューターの皆さんからどっちに揃えていいかわからないという声は当然あるわけで。昔はそういう時代じゃなかったけど、今のGitHub時代では「最低限のコーディング規約はあったほうがいいんですかねぇ」とは思います。

そして、それをやっているのがRuby on Railsプロジェクトですね。

Railsのコードスタイル

ーRailsプロジェクトでは、コーディング規約があってRuboCopで検査しているわけですね。

はい。本当に最低限の所だけ機械にやらせるみたいな感じで。RuboCopデフォルトのルールは無視してしまって、自分たちで「最低限ここだけは」というルールを入れています。

ー最低限のルールというのはどんな感じですか?

例えば、有名なDHH Indentationがあります。privateのときにそこから下のコードを全部右にずらすっていうキモいやつ。 IndentationConsistency = rails これは名前が rails になっちゃってることからわかるように、Rails以外では見たことないですね。プロジェクトオーナーが頑なに主張するので、今はこれが採用されています。

あと僕が最高に気に入らないのが StringLiterals = double_quotes。つまり「文字列のクォート記号は全部ダブルクォート」。これはDHHじゃなくて誰かが雑に入れちゃったんですよ。既存のコードを片っ端から書き換えてくれちゃって。こんなどうでもいい理由でそういうことをされるとgit blame が汚れるんでほんと困るんですよねぇ。

あとは RequireParentheses = true も気に入らないです。メソッド呼び出しはカッコをつけてください。これは要らないと思うんだけどなぁ、Javaじゃないんだし。

あ、Railsでは SpaceInsideHashLiteralBraces = true で、Hashのリテラルの中にスペースを空けろなんですよ。これはマイノリティーだと思うんだけどなぁ。なんかHashってキュっと書きたいじゃないですか。なので、僕はスカスカして気持ち悪いなぁ、って思いながら書いてます。

要は、自分が仕切ってるプロジェクトだったら自分のスタイルで書きますけど、他人が書いてるコードのスタイルがちょっとぐらい自分と違ってもあまりに気にならないし、さらに言うとリポジトリーオーナーから「これでよろしく」というルールが提示されてるんであれば、僕は少なくともそれに従いますね。

Pull Requestを出す場合でも一緒です。既存のコードをじっくり読んで、なるべくそこのコードの文脈を読み取って、「きっとここはこういう風なルールでやってるんだろうな」というのを推測してそれに合わせたコミットをする、というのを心がけてるんです。そこは空気を読むっていうか。それもまたコードを読むときの楽しみの1つだと思っているし、そんなふうにして違和感の無いコミットを作る技術もOSSディベロッパーとしての大事なスキルのうちの1つだと思っています。

ー意外とたくさんルールがあるんですね。RuboCop導入の経緯とかルールの決め方とかはどんな感じですか?

やっぱりPull Requestのレビューの所で、「ここにTrailingWhitespaceが入ってます」みたいな不毛なやりとりは人間がやらなくてもいいようにしてる、っていうのが最初だったはずです。

どういうルールを採用するかというのは、チャットとかで誰かが提案して、「いいんじゃない」ってなれば入るとかそれぐらいですね。今見ると、いつの間にか結構増えてますね。最初は2、3個だったんですけどね。

一番最初のコミットを見ると面白いかもしれないですね。

https://github.com/rails/rails/blob/7d883b829307944f6d7c273f3915ee7059ee3771/activerecord/.rubocop.yml

だいぶ今よりシンプルですよね。この頃から括弧は必要とか、Style/Hash、Style/TrailingWhitespace、あとは2スペースノータブ。

最初はこれくらいから始まって、僕もこれくらいなら悪くないと思ってたんですけど、結構いつのまにか増えてますね。正直めんどくさいです。

これはやり方としては凄く参考になる部分がある気がしますね。実際に仕事のプロジェクトなんかに導入しようとしたときには、最低限「これはみんな許容できるよね」ってところから始めて、少しずつルールを追加していくという。RuboCopみたいなものに抵抗感ある人も多いので、いつのまにかRuboCopをじわじわと浸透させるには良いプラクティスじゃないでしょうか。

標準コーディング規約

ーGitHubとかクックパッドとか、コーディング規約を決めていて、公開までしている企業もありますね。

それを好む好まざるはともかく、そういう部分が決まってる会社は業務としては効率よくやろうと工夫して取り組んでる現場なんだろうなという印象はありますね。規約が公開されていると、もしかしたらそこに入社しようか考えている人が、「この規約は絶対に従えないから」と思って、最初から合わない企業に当たらないで済む可能性があるかもしれないですね。

ーコーディング規約があるおかげで効率が上がる感じはありますか?

正直、他人のコードが自分のスタイルとちょっと違っても気にしない社員ばっかりだと、コーディング規約がなくてもそれはそれで回ると思うんですけど。コードレビューでそういうのを指摘して仕事した気になっちゃうみたいな人がいるじゃないですか。そこを防ぐ効果はありそうだから、確かに価値はあるかもしれません。

「Hashのブレースの中はスペースを空けてください」とか、僕はどっちでもいいじゃん動くじゃんとか思うんだけど、そのやりとりをする時間を省略できるということですね。

1つ思うのは、いわゆる標準コーディング規約みたいなのがコミュニティー全体に、あった方が幸せなのかなあと。RuboCopのアレではなくて、ちゃんとRubyっぽいやつ。

昔は前田修吾さんのNaClコーディング規約がありましたね。あれは今となっては……という感じで、古すぎる感じがしますけど。80文字ルールを厳格に、とか、ないわーと思います。あとは青木さんのもありましたよね。改行をいっぱい空けろとかそこが印象に残ってます。すきま重要って言ってるんですよね。これは読み物として抜群に面白いですよ、すごく味があって。

ー標準の規約と言えば、Elixirは次のバージョンで標準コードフォーマッタまで提供することにしたと聞いたんですが。

最近の実装で、José本人ではなくて誰か他の人の提案だそうです。Goみたいなノリですよね。言語本体に標準コードフォーマッターが入るのは。

ー言語本体に入れるんなら、言語自体を変えてしまえば良いと思うんですが。

それをやると大きな非互換になっちゃうので。自由ではあるんだけど、標準のフォーマットは提供する。

Joséに聞いた話では、「そんなのはElixirらしくない」と最初は思っていたんだけど、提案者があまりにも猛烈にプッシュするので、「じゃあ」と入れてみたと。入れてみたら一発で気に入ってしまって、「最初からそうしとけばよかったと今は思ってるよ」って言ってました。

「そんなのを他人に強制されるのは気に食わない、ってRubyistは思ってるかもしれないけど、強制されるてみると楽になっていいよ」というふうにJoséは言ってましたよ。まぁ、Rubyが標準フォーマットを取り入れるとかはないとは思いますけど、1つ興味深い話ではありますよね。

ー松田さん自身のプロジェクトでは、RuboCopは使ってないんですか?デフォルトのルールはともかくとして、Pull Requestのコードを機械的に検査したいみたいなことはあるんじゃないかと思いますが。

RuboCopは自分のOSSプロジェクトでは使ってないですね。今のところ、スタイルに関する議論が面倒だと感じる規模でやってないから、その必要性を感じてないだけかもしれません。

Pull Requestをもらった時に対処する方法っていくつかあって、たとえばRailsみたいにレビューコメントで最高のPull Requestになるまでじっくり育ててからマージするという方法もあるんですが、僕はそのやりとりを省略しちゃって一旦masterに入れちゃってから自分で好みのコードに書き直す、ということが割とありますね。

たとえばKaminariくらいの規模だと、とりあえず、はいはいってマージしちゃうんですよ。それで後でコードをいじって書き換えるわけです。でもそれは、スタイルよりはロジックとかの部分が多いですかね。

あ、でも同じ事を須藤さんにやられた時は、「だったらPull Requestのときに言ってよ」と思ったので、もしかしたら自分ではやるけど他人にはやられたくないのかもしれないです。ワガママですね。

これがそのPull Requestなんですけど、 $LOADED_FEATURES の文字列を書き換えるとかいうありえない感じのsuper ugly hackみたいなやつを、「こうしないと動かないんですよ」っていう説明をしつつ「本当、汚いコードでごめんなさい」って言ってPull Requestで出しました。そしたら「It’s surprising solution」っていうお褒めのお言葉をいただいて、それでマージしてもらった後に須藤さんの好みのコードに書き換わっていましたっていう。

コードレビュー自動化とSideCI

f:id:sideci:20180514132852j:plain

僕の経験では一回、すごい細かい .rubocop.yml を採用してみたっていうお仕事のプロジェクトがありました。それはそれで .rubocop.yml の内容を決める作業にすごい時間を取られて、生産性下がってるじゃん、ってことになって、結局何もない状態に戻したということがあります。

やっぱり DisabledByDefault ですよ。そうじゃないと、RuboCopのバージョンが上がると、まず bundle update が通らないんですよね。毎度毎度新しいルールが増えすぎて。

これではプロダクトのデイリーの bundle update の妨げになるわけで、良くない気がしますね。開発生産性を上げるために入れてるのに、むしろ足を引っ張ってるっていう。

ーその問題は、SideCIを使うと改善するかもしれませんね。 それはCIでRuboCopを実行しているから起きる問題で、CIから切り離してSideCIにRuboCopを任せればそういうことは起きません。SideCIでは、Pull Requestで変更された部分だけが対象になるので。

そうすると、自分のせいじゃない変更で、いつか誰かが埋めてくれた地雷を後から踏む可能性があるということですよね。まあ、それは git blame の犯人になるはずの人が責任を持って誰にも怒られないコードをコミットをすればいいということか。「これ壊したの俺じゃないのに」とかブツブツ言いながら。

ーもしくは、そもそも直さないか。SideCIはRuboCopが検査して出てきた警告を、もう一度人間がレビューして受け入れるかどうか決めるという風になっています。RuboCopと意見が合わない時は、とりあえず無視して先に進むってことができるようになってるんですよ。

それは技術的に言うと、RuboCopをignoreするコメントを自動で挿入してくれるということですか?それともSideCI側でデータベースを持っている?

ーデータベースをSideCI側で持ってます。コミットをまたいで見るので、「この無視された警告はこの行に移動して」みたいなこともトラッキングしています。 これで、RuboCopのちょっと怪しいルールなんかもガンガン有効にできるようになります。RuboCopと意見が合わないこともありますが、役に立つルールもあるので。

そうですね。確かに DisabledByDefault の場合は、例えばセキュリティ関連とか役に立つルールが入ってくれた時には気づけないってのは問題としてはありますね。

いや、さすがにちゃんと色々と考えて作り込まれてるんだなと理解しました。

RubyKaigi 2018

ー今年のRubyKaigiには、RuboCopの作者がスピーカとして参加するそうですね。RuboCopの話をしてくれるとプログラムにあります。

はい。bbatsovが今回初来日で、RubyKaigi初日のスピーカーとなっています。これはRuby 3みたいな話とは別の、今年のRubyKaigiの1つの注目ポイントですね。コーディングスタイルの話とかRuboCopがどうだ、とかいう話はRubyコミッターの間でも話題になることは多いんで、RuboCop作者本人がやってきて日本のコミッターたちと意見交換ができるというのはなかなか有意義なんじゃないでしょうか。

彼は去年、EuRoKoというヨーロッパのカンファレンスでキーノートをしていて、彼なりに将来のRubyのデザインについて話をしているんですが、ちょっと正直言ってRubyの良さを何もわかってない感じだったので、そのへんについて直接コミッターたちと話ができるのは彼にとってもいい機会になるはずです。

EuRuKo 2017 https://euruko2017.org Ruby4: To infinity and Beyond https://speakerdeck.com/bbatsov/ruby-4-to-infinity-and-beyond

ちなみに、彼の今回のプロポーザルには「話したいことがいっぱいありすぎて、40分の枠で何を話したら良いか迷っている」というふうに書いてくれていたので、僕の方からフィードバックとして「RubyKaigiは開発者のための技術的なことにフォーカスしたカンファレンスだから、テクニカルな話にしてくれ」という要望を伝えました。ので、今回はもうちょっと実装寄りの、面白い話が聞けるんじゃないでしょうか。

f:id:sideci:20180514135959j:plain

RubyKaigiには、SideCIからは私 (@soutaro) と @pocke が発表します。既に開催の直前ですので、なかなかRubyKaigiに参加しようというきっかけにはならないと思いますが、国内からならまだ間に合うはずですので、ぜひ皆さんも参加をご検討ください!

RuboCop vs Rails Best Pratices それぞれの特徴。初心者はどう使う?

目次

開発をする際にテストを利用しする事は、今日最早あたりまえになったと思います。 また、GitHubやBitBucketといったWebサービスの普及により、merge前にcode reviewやテストを行なう事もあたりまえになったと思います。

それでは、テストで担保出来ない点はどうでしょう。例えば

  • コードの複雑性
  • 変数名の妥当性
  • フレームワークに沿ったコーディング
  • 紛らわしい評価のされるコード
  • etc.

などはテストでは担保されませんし、reviewerの負担になります。

では、どのようにしてその負担を減らす事が出来るでしょうか。

この記事では、Ruby on Rails のプロダクトをターゲットに、これらの unit testを走らせる前にreviewの支援を行なうツール について論じてみようと思います。

どのようなツールがあるか

現在、Rubyにおいては各種そのような支援プロダクトがあると思いますが、今回は利用者も多いと考えられる、

を試していきます。

RuboCop

特徴

Copと飛ばれる各種検査モジュールがあり、それらが表記揺れであったり、文法であったり、methodの複雑性の検査を行ないます。 また、 .rubocop.yml と呼ばれるファイルでそれらの on / off, 閾値の設定などが可能となっています。 さらに、 auto-correct という機能があり、RuboCopが判断出来る範囲で規約通りに修正してくれます。

rails_best_practices

特徴

rails-bestpractices.com というrails applicationを実装する際のベストプラクティス集サイトがあります。このベストプラクティスに沿っているかを検査する機能を持ちます。

Ruby on RailsのMVCの静的解析などが可能となっています。 また Ruby on Rails標準のORMである ActiveRecord 以外にも mongoidmongomapper にも対応しています。 template engineも erb はもちろん、 slim, haml, rabl 等豊富です。

試してみる。

対象

  • ruby 2.4.2
  • ターゲットはrailsアプリの代表格で、gem / rails_best_practices を採用していない Redmineの3.4.3を対象とします。
  • redmineを動作させる事は目的としませんので、起動までは確認しません。

RuboCop

導入

Gemfileの書き換え

group :test

gem 'rubocop', require: false

を追加します。

database.ymlの作成

bundle install 時にGemfileでdatabase.ymlを見ているので追加します。

production: &production
  adapter: sqlite3
  database: db/redmine.sqlite3

development:
  <<: *production

test:
  <<: *production
Gemの導入
$ bundle install --path=vendor/bundle --binstubs

環境によっては事前にImageMagick6を入れておく必要があります。

作動させてみる

$ bin/rubocop -P

Pオプションで並列実行されます。rubocopは比較的重い処理の為、CPUが多数ある環境では付けておくと良いでしょう。

さて結果は、

以上略

db/migrate/20110226120112_change_repositories_password_limit.rb:1:1: C: Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
class ChangeRepositoriesPasswordLimit < ActiveRecord::Migration
^
db/migrate/20110226120112_change_repositories_password_limit.rb:3:54: C: Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => nil, :default => ''
                                                     ^^^^^^^^^
db/migrate/20110226120112_change_repositories_password_limit.rb:3:69: C: Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => nil, :default => ''
                                                                    ^^^^^^^^^^^
db/migrate/20110226120112_change_repositories_password_limit.rb:3:81: C: Metrics/LineLength: Line is too long. [82/80]
    change_column :repositories, :password, :string, :limit => nil, :default => ''
                                                                                ^^
db/migrate/20110226120112_change_repositories_password_limit.rb:7:54: C: Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => 60, :default => ''
                                                     ^^^^^^^^^
db/migrate/20110226120112_change_repositories_password_limit.rb:7:68: C: Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => 60, :default => ''
                                                                   ^^^^^^^^^^^
db/migrate/20110226120112_change_repositories_password_limit.rb:7:81: C: Metrics/LineLength: Line is too long. [81/80]
    change_column :repositories, :password, :string, :limit => 60, :default => ''
                                                                                ^

979 files inspected, 64504 offenses detected

読者のみなさんを驚かせてしまったかもしれませんが、細かい設定をしないでrubocopを作動させると、非常に多数の違反を検知し、今回は64504件の違反(ちなみに原文のoffenseは犯罪の意味もあり、cop(警官)ならではのジョークを感じます)という結果となりました。

failに対応してみる

上記のOffenceの大まかな理由は、

  1. Stringをfrozen指定するCommentが入っていない(Style系Cop)
  2. hashのsyntaxがruby 1.9系の書式(Style系Cop)
  3. 1行が長すぎる(Metrics系Cop)

になります。

まずは、これらに対応してみましょう。

diff --git a/db/migrate/20110226120112_change_repositories_password_limit.rb b/db/migrate/20110226120112_change_repositories_password_limit.rb
index 1ad937c7d..0d244f031 100644
--- a/db/migrate/20110226120112_change_repositories_password_limit.rb
+++ b/db/migrate/20110226120112_change_repositories_password_limit.rb
@@ -1,9 +1,10 @@
+# frozen_string_literal: true
 class ChangeRepositoriesPasswordLimit < ActiveRecord::Migration
   def self.up
-    change_column :repositories, :password, :string, :limit => nil, :default => ''
+    change_column :repositories, :password, :string, limit: nil, default: ''
   end

   def self.down
-    change_column :repositories, :password, :string, :limit => 60, :default => ''
+    change_column :repositories, :password, :string, limit: 60, default: ''
   end
 end

そして再実行。

$ bin/rubocop db/migrate/20110226120112_change_repositories_password_limit.rb
Inspecting 1 file
C

Offenses:

db/migrate/20110226120112_change_repositories_password_limit.rb:2:1: C: Layout/EmptyLineAfterMagicComment: Add an empty line after magic comments.
class ChangeRepositoriesPasswordLimit < ActiveRecord::Migration
^
db/migrate/20110226120112_change_repositories_password_limit.rb:2:1: C: Style/Documentation: Missing top-level class documentation comment.
class ChangeRepositoriesPasswordLimit < ActiveRecord::Migration
^^^^^

1 file inspected, 2 offenses detected

修正した事により、他の問題が出たようです。

  • MagicComment後に空行が入ってない。(Layout系Cop)
  • TopLevelのclassに対してドキュメントが記述されていない。(Style系Cop)

上記ではさらに細かい部分が検知されていますが、これらも検知されないように、さらに対応してみましょう。

diff --git a/db/migrate/20110226120112_change_repositories_password_limit.rb b/db/migrate/20110226120112_change_repositories_password_limit.rb
index 1ad937c7d..29c91ad56 100644
--- a/db/migrate/20110226120112_change_repositories_password_limit.rb
+++ b/db/migrate/20110226120112_change_repositories_password_limit.rb
@@ -1,9 +1,12 @@
+# frozen_string_literal: true
+
+# @class Abracadabra
 class ChangeRepositoriesPasswordLimit < ActiveRecord::Migration
   def self.up
-    change_column :repositories, :password, :string, :limit => nil, :default => ''
+    change_column :repositories, :password, :string, limit: nil, default: ''
   end

   def self.down
-    change_column :repositories, :password, :string, :limit => 60, :default => ''
+    change_column :repositories, :password, :string, limit: 60, default: ''
   end
 end

そして再実行。

$ bin/rubocop db/migrate/20110226120112_change_repositories_password_limit.rb
Inspecting 1 file
.

1 file inspected, no offenses detected

ようやくrubocopを納得させました。

ちなみに --auto-correct オプションを使う事により、殆どの内容を自動修正する事が可能でした。

$ bin/rubocop -a db/migrate/20110226120112_change_repositories_password_limit.rb
Inspecting 1 file
C

Offenses:

db/migrate/20110226120112_change_repositories_password_limit.rb:1:1: C: [Corrected] Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
class ChangeRepositoriesPasswordLimit < ActiveRecord::Migration
^
db/migrate/20110226120112_change_repositories_password_limit.rb:2:1: C: [Corrected] Layout/EmptyLineAfterMagicComment: Add an empty line after magic comments.
class ChangeRepositoriesPasswordLimit < ActiveRecord::Migration
^
db/migrate/20110226120112_change_repositories_password_limit.rb:3:1: C: Style/Documentation: Missing top-level class documentation comment.
class ChangeRepositoriesPasswordLimit < ActiveRecord::Migration
^^^^^
db/migrate/20110226120112_change_repositories_password_limit.rb:3:54: C: [Corrected] Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => nil, :default => ''
                                                     ^^^^^^^^^
db/migrate/20110226120112_change_repositories_password_limit.rb:3:69: C: [Corrected] Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => nil, :default => ''
                                                                    ^^^^^^^^^^^
db/migrate/20110226120112_change_repositories_password_limit.rb:7:54: C: [Corrected] Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => 60, :default => ''
                                                     ^^^^^^^^^
db/migrate/20110226120112_change_repositories_password_limit.rb:7:68: C: [Corrected] Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => 60, :default => ''
                                                                   ^^^^^^^^^^^

1 file inspected, 7 offenses detected, 6 offenses corrected

設定ファイルでコーディングスタンダードを定める

.rubocop.yml を設定をする事により、重要視しないCopを無効化したり、Metrics系等の閾値を調整したりする事が可能です。

プロジェクトは規約を遵守する為でなく、効率良く開発する為に規約が存在すると私は考えるので、この全てを押し付けず調整出来る機能はありがたいです。

先程のsourceを元に戻し、以下の内容で rubocop.yml を作ってみましょう。ただし、この内容は私が妥当と思う内容では決して無く、あくまでテスト用です。

Style/Documentation:
  Enabled: false
  
Layout/EmptyLineAfterMagicComment:
  Enabled: false

Metrics/LineLength:
  Max: 100 

実行結果は以下の通りです。

$ bin/rubocop  db/migrate/20110226120112_change_repositories_password_limit.rb
Inspecting 1 file
C

Offenses:

db/migrate/20110226120112_change_repositories_password_limit.rb:1:1: C: Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
class ChangeRepositoriesPasswordLimit < ActiveRecord::Migration
^
db/migrate/20110226120112_change_repositories_password_limit.rb:3:54: C: Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => nil, :default => ''
                                                     ^^^^^^^^^
db/migrate/20110226120112_change_repositories_password_limit.rb:3:69: C: Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => nil, :default => ''
                                                                    ^^^^^^^^^^^
db/migrate/20110226120112_change_repositories_password_limit.rb:7:54: C: Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => 60, :default => ''
                                                     ^^^^^^^^^
db/migrate/20110226120112_change_repositories_password_limit.rb:7:68: C: Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
    change_column :repositories, :password, :string, :limit => 60, :default => ''
                                                                   ^^^^^^^^^^^

1 file inspected, 5 offenses detected

設定したCopが外れていますね。

実際どのような設定項目があるかは、一番簡単な方法はRuboCopの

  • config/disaled.yml
  • config/enabled.yml

を見る事でしょう。

対応しているチェック種類

rubydocを見るのが速いでしょう。

Layout等の表記揺れから、Lintなどのバグの温床チェック、Security周り、Railsまで、かなり多岐に渡ります。

少し覗いてみると、

  • Layout
    • インデントなどのレイアウトに関するCop
  • Lint
    • 文法的に間違いでは無いが、紛らわしい書式に関するCop
  • Metrics
    • classやmethodの複雑度に関するCop
  • Naming
    • methodや定数、ファイル名などの命名に関するCop
  • Performance
    • 性能に関するCop
  • Rails
    • Rails特有の記述に関するCop
  • Security
    • セキュリティホールになりやすい記述に関するCop
  • Style
    • 細かめの記述方法に関するCop
  • Bundler
    • bundlerのGemfile等の書式に関するCop
  • Gemspec
    • Gemspecの記述に関するCop

などがあります。

触れてみた感触

上記でも述べたように、チェック項目がかなり多岐に渡っています。また、開発活発度もかなりのものです。

ただ、基本方針として、かなり固い方向に初期設定を振っているので、開発内で意見調整をしながら.rubocop.ymlを随時チューニングする必要があると感じました。

auto-correctによる自動修正は非常に強力で、開発者からcoding時のわずらわしさから相当開放し、レビュー時に、もっと本質を見る事が可能になりそうです。

多用はあまり推奨しませんが、前述のように規約を守ることよりも効率よく開発することを優先したい場合に使える、部分的にチェックを無効にする方法も、簡単に記載します。

# rubocop:disable Metrics/MethodLength, Metrics/AbcSize
def foo
    ながーーーいmethod
end
# rubocop:enable Metrics/MethodLength, Metrics/AbcSize

rails_best_practices

導入

Gemfileの書き換え

group: test

 gem  'rails_best_practices'

を追加します。

SublimeText2やTextmate2との連動

筆者は生粋のvimmerの為、詳しくは調査していませんが、下記のように各editorと連携出来るようです。

  • handlerを追加する事により、SublimeText2とシームレスに使えるようです。
  • tmbundleを追加する事により、Textmate2とシームレスに使えるようです。
databases.ymlの作成とGemの導入

rubocopと同様です。

production: &production
  adapter: sqlite3
  database: db/redmine.sqlite3

development:
  <<: *production

test:
  <<: *production
$ bundle install --path=vendor/bundle --binstubs

作動させてみる

以下いくつか抜粋。

$ bin/rails_best_practices .
Source Code: |===================================================================================================================================================================|
/Users/foobar/vcswork/redmine/app/models/enumeration.rb:21 - default_scope is evil
/Users/foobar/vcswork/redmine/db/migrate/007_create_journals.rb:34 - isolate seed data
/Users/foobar/vcswork/redmine/app/controllers/account_controller.rb:61 - move model logic into model (@token use_count > 4)
/Users/foobar/vcswork/redmine/app/views/imports/settings.html.erb:9 - move code into helper (array_count >= 3)
/Users/foobar/vcswork/redmine/app/views/wiki/rename.html.erb:14 - remove trailing whitespace

Please go to https://rails-bestpractices.com to see more useful Rails Best Practices.

Found 1316 warnings.

なにも設定しないと全部で1316件のwarningを検知したようです。これはrubocopよりは少ないと言えます。 が、move code into modeldefault_scope is evil など、railsに特化したメッセージが見受けられます。

メッセージの詳細や対処方法は、 rails-bestpractices.comを参考にとの事。

少し面白い機能として、 $ bin/rails_best_practices -f html . するとrails_best_practices_output.htmlが出力され、warning種別毎にリストを表示したりする事が可能になります。

warningに対応してみる

上記のwarningの大まかな理由は、

  1. default_scope is evil : default scopeを使っている。
  2. isolate seed data : migrationにseedを書いている。
  3. move model logic into model : いわゆるfat controller
  4. move code into helper : viewの複雑なcodeをhelperへ逃がせてない。
  5. remove trailing whitespace : 不要な行末スペースがある。

になります。各warningに関するページは warning名 rails-bestpractices で検索しましょう。

それではこれらに対応してみましょう。

default scope is evil

ActiveRecordのdefault scopeを使っています。これは要件によって対応方法が変わるので、画一的に対応は不可能です。 ただし、以下の理由により、利用する事は好ましくないと言えましょう。(詳細は上記のweb page参照の事)

  • unscopeを使って解除しない限り、default scopeをoverrideする事が出来ない。
  • modelのinitializationに影響を与えてしまう。
isolate seed data

migration内でrecordをイジるのは、recordの宣言箇所の一貫性の意味で好ましくありません。schema変更とseed dataの投入は分けましょう。

具体的にはrecordの挿入は db/seeeds.rb に任せましょう。

$ git diff --cached
diff --git a/db/migrate/007_create_journals.rb b/db/migrate/007_create_journals.rb
index 63bdd2374..6fa28c61c 100644
--- a/db/migrate/007_create_journals.rb
+++ b/db/migrate/007_create_journals.rb
@@ -27,13 +27,6 @@ class CreateJournals < ActiveRecord::Migration

     Permission.create :controller => "issues", :action => "history", :description => "label_history", :sort => 1006, :is_public => true, :mail_option => 0, :mail_enabled => 0

-    # data migration
-    IssueHistory.all.each {|h|
-      j = Journal.new(:journalized => h.issue, :user_id => h.author_id, :notes => h.notes, :created_on => h.created_on)
-      j.details << JournalDetail.new(:property => 'attr', :prop_key => 'status_id', :value => h.status_id)
-      j.save
-    }
-
     drop_table :issue_histories
   end

diff --git a/db/seeds.rb b/db/seeds.rb
new file mode 100644
index 000000000..f8f66c2f5
--- /dev/null
+++ b/db/seeds.rb
@@ -0,0 +1,8 @@
+# data migration
+IssueHistory.all.each {|h|
+  j = Journal.new(:journalized => h.issue, :user_id => h.author_id, :notes => h.notes, :created_on => h.created_on)
+  j.details << JournalDetail.new(:property => 'attr', :prop_key => 'status_id', :value => h.status_id)
+  j.save
+}
move model logic into model

本来modelに寄せるべきロジックがcontrollerに寄っています。いわゆるFat Controllerです。業務ロジックは業務ModelとしてModelに寄せましょう。

warningになっているapp/controllers/account_controller.rb:61lost_passwordは下記に示しますが、実際長く、メッセージ通り、 @token が4回以上使われています。

ここのrefactoringには @token もですが、そこから取り出されている @user も込みで改修を考える必要があるでしょう。

def lost_password
  (redirect_to(home_url); return) unless Setting.lost_password?
  if prt = (params[:token] || session[:password_recovery_token])
    @token = Token.find_token("recovery", prt.to_s)
    if @token.nil? || @token.expired?
      redirect_to home_url
      return
    end

    # redirect to remove the token query parameter from the URL and add it to the session
    if request.query_parameters[:token].present?
      session[:password_recovery_token] = @token.value
      redirect_to lost_password_url
      return
    end

    @user = @token.user
    unless @user && @user.active?
      redirect_to home_url
      return
    end
    if request.post?
      if @user.must_change_passwd? && @user.check_password?(params[:new_password])
        flash.now[:error] = l(:notice_new_password_must_be_different)
      else
        @user.password, @user.password_confirmation = params[:new_password], params[:new_password_confirmation]
        @user.must_change_passwd = false
        if @user.save
          @token.destroy
          Mailer.password_updated(@user)
          flash[:notice] = l(:notice_account_password_updated)
          redirect_to signin_path
          return
        end
      end
    end
    render :template => "account/password_recovery"
    return
  else
    if request.post?
      email = params[:mail].to_s
      user = User.find_by_mail(email)
      # user not found
      unless user
        flash.now[:error] = l(:notice_account_unknown_email)
        return
      end
      unless user.active?
        handle_inactive_user(user, lost_password_path)
        return
      end
      # user cannot change its password
      unless user.change_password_allowed?
        flash.now[:error] = l(:notice_can_t_change_password)
        return
      end
      # create a new token for password recovery
      token = Token.new(:user => user, :action => "recovery")
      if token.save
        # Don't use the param to send the email
        recipent = user.mails.detect {|e| email.casecmp(e) == 0} || user.mail
        Mailer.lost_password(token, recipent).deliver
        flash[:notice] = l(:notice_account_lost_email_sent)
        redirect_to signin_path
        return
      end
    end
  end
end
move code into helper

view層に対して複雑なロジックや大きめなコードがあります。viewの見通しが悪くなる為、helperに寄せましょう。

$ git diff
diff --git a/app/helpers/imports_helper.rb b/app/helpers/imports_helper.rb
index 39f3b95ab..934b1d262 100644
--- a/app/helpers/imports_helper.rb
+++ b/app/helpers/imports_helper.rb
@@ -44,4 +44,12 @@ module ImportsHelper
       [format, f]
     end
   end
+
+  def formatting_for_options_separator(var)
+    options_for_select([[l(:label_comma_char), ','], [l(:label_semi_colon_char), ';']], var.settings['separator'])
+  end
+
+  def formatting_for_options_wrapper(var)
+    options_for_select([[l(:label_quote_char), "'"], [l(:label_double_quote_char), '"']], var.settings['wrapper'])
+  end
 end
diff --git a/app/views/imports/settings.html.erb b/app/views/imports/settings.html.erb
index 374dba5b1..c00ffbcfb 100644
--- a/app/views/imports/settings.html.erb
+++ b/app/views/imports/settings.html.erb
@@ -5,13 +5,11 @@
     <legend><%= l(:label_options) %></legend>
     <p>
       <label><%= l(:label_fields_separator) %></label>
-      <%= select_tag 'import_settings[separator]',
-            options_for_select([[l(:label_comma_char), ','], [l(:label_semi_colon_char), ';']], @import.settings['separator']) %>
+      <%= select_tag 'import_settings[separator]', formatting_for_options_separator(@import) %>
     </p>
     <p>
       <label><%= l(:label_fields_wrapper) %></label>
-      <%= select_tag 'import_settings[wrapper]',
-          options_for_select([[l(:label_quote_char), "'"], [l(:label_double_quote_char), '"']], @import.settings['wrapper']) %>
+      <%= select_tag 'import_settings[wrapper]', formatting_for_options_wrapper(@import) %>
     </p>
     <p>
       <label><%= l(:label_encoding) %></label>
remove trailing whitespace

不要な末尾のスペースが存在しています。

$ git diff
diff --git a/app/views/wiki/rename.html.erb b/app/views/wiki/rename.html.erb
index ac88fd4bf..602850d13 100644
--- a/app/views/wiki/rename.html.erb
+++ b/app/views/wiki/rename.html.erb
@@ -11,7 +11,7 @@
 <p><%= f.text_field :title, :required => true, :size => 100  %></p>
 <p><%= f.check_box :redirect_existing_links %></p>
 <p><%= f.select :parent_id,
-                content_tag('option', '', :value => '') + 
+                content_tag('option', '', :value => '') +
                   wiki_page_options_for_select(
                     @wiki.pages.includes(:parent).to_a - @page.self_and_descendants,
                     @page.parent),

その他

列挙したのは極く一部のものです。railsを構成する、Model / View / Controller から細かいhelper、routingなど多岐に亘ります。

一度 https://rails-bestpractices.com/ を見てみるのも良いかもしれません。

設定ファイルを調整してみる

設定ファイルを作成する事により、解析内容をカスタマイズする事が可能です。

まず雛形を作りましょう。 $ bin/rails_best_practices -g を実行すると config/rails_best_practices.yml が生成されます。

AddModelVirtualAttributeCheck: { }
AlwaysAddDbIndexCheck: { }
#CheckSaveReturnValueCheck: { }
............
............
MoveCodeIntoHelperCheck: { array_count: 3 }
MoveCodeIntoModelCheck: { use_count: 2 }
MoveFinderToNamedScopeCheck: { }
MoveModelLogicIntoModelCheck: { use_count: 4 }
NeedlessDeepNestingCheck: { nested_count: 2 }
............
............
RemoveUnusedMethodsInControllersCheck: { except_methods: [] }
RemoveUnusedMethodsInHelpersCheck: { except_methods: [] }
RemoveUnusedMethodsInModelsCheck: { except_methods: [] }
ReplaceComplexCreationWithFactoryMethodCheck: { attribute_assignment_count: 2 }
............
UseScopeAccessCheck: { }
UseTurboSprocketsRails3Check: { }

不要なテストがある場合、#や行を消す事により、対象テストをキャンセルする事が可能となります。

また、defaultで入っている属性(オプション)の値を替えたり、配列に値を追加する事により、Checkのカスタマイズが可能となります。

たとえば

MoveModelLogicIntoModelCheck: { use_count: 4 }

use_count5 にすれば、controller内でmodelのあるModelの利用回数が5回以上の場合にwarningになるようになります。

また、全てのCheckには ignore_files オプションを設定する事が可能で、

DefaultScopeIsEvilCheck: { ignored_files: 'user\.rb' }
LongLineCheck: { max_line_length: 80, ignored_files: ['db/migrate', 'config/initializers'] }

のように、1エントリか、配列を受ける事が可能になっています。

対応しているチェック種別

  • Move code from Controller to Model
    • Move finder to named_scope (rails2 only)
    • Use model association
    • Use scope access
    • Add model virtual attribute
    • Replace complex creation with factory method
    • Move model logic into the Model
    • Check return value of "save!"
  • RESTful Conventions
    • Overuse route customizations
    • Needless deep nesting
    • Not use default route
    • Restrict auto-generated routes
  • Model
    • Keep finders on their own model (rails2 only)
    • The law of demeter
    • Use observer
    • Use query attribute
    • Remove unused methods in models
    • Protect mass assignment
    • Destroy return value (disabled by default)
  • Mailer
    • Use multipart/alternative as content_type of email
  • Migration
    • Isolating seed data
    • Always add database index
    • Use say with time in migrations
  • Controller
    • Use before_filter (disabled by default)
    • Simplify render in controllers
    • Remove unused methods in controllers
  • Helper
    • Remove empty helpers
    • Remove unused methods in helpers
  • View
    • Move code into controller
    • Move code into model
    • Move code into helper
    • Replace instance variable with local variable
    • Simplify render in views
    • Not use time_ago_in_words
  • Deployment
    • Dry bundler in Capistrano
    • Speed up assets precompilation with turbo-sprockets-rails3
  • Other
    • Remove trailing whitespace
    • Remove tab (disabled by default)
    • Hash syntax (disabled by default)
    • Use parentheses in method definition (disabled by default)
    • Long line (disabled by default)
    • Not rescue exception

触れてみた感触

Railsに関しては、MVCからmigration、helper、capistranoなど、ほとんどを含んでおり、かなりリッチな作りになっているように思われる。

ただし、 開発はあまり活発とは言えず、また、モトネタとなる Rails Bestpractices 自体も更新が止まっているようです。

とは言うものの、現状、提供されているCheckはRails5系でも有用に思われます。

結論

両方使え

筆者の個人的な感想を含んだ結論ですが、 RuboCop はrubyのcodeの静的解析を主に、rails_best_practices はその上に乗るrailsの静的解析を主に置いたツールと言えるでしょう。 ようするに 対象とするレイヤーが異なる のです。

双方のツールを導入し、それぞれのレイヤーのテストを重なる部分はあれど、補完しあえば良いかと考えます。

重要な点は、 解析ツールに合わせたコードを書くのでは無く、プロジェクトに必要とされるコードを素早く書く という事だと思います。ツールの結果に合わせてコードを曲げる(読みにくくしたり、リファクタリングの際にエンバグしたり)のは本末転倒と言えるでしょう。

これらの運用方法の検討に時間をかける余裕がない場合、 SideCI 等のcloud solutionを考えてみるのも良いかもしれません。SideCIではgithubやslackと連動させ、静的解析の結果をreviewer/revieweeへ通知し、reviewを行なう環境を簡単に得る事が出来ます。

リポジトリ数無制限の新しい料金プランの提供を開始。旧プランからの移行のお願い

こんにちは。SideCIの角です。いつもSideCIをご利用いただきありがとうございます。

SideCIは料金プランを2018年5月1日より新しい体系に改定いたします。新しい料金プランでは、

  • プライベートリポジトリ数ではなくユーザ数に応じた料金体系となり、開発チームの規模に応じて料金が変動する
  • アップグレード、ダウングレードの際に、利用料金の変更が日割り計算される

といった特徴がありますので、新プランへの移行をお願いします。(すでにご契約いただいているプランは当面の間そのままお使いいただけますのでご安心ください。)

料金プランの改定内容

旧プラン(2018年4月30日まで)

  • ユーザ数: 無制限
  • プライベートリポジトリ数: 解析が可能なプライベートリポジトリ数をプランに応じて制限
  • パブリックリポジトリ: 無制限
  • プラン: マイクロプラン3,200円/月(税込み)から提供

新プラン(2018年5月1日から)

  • ユーザ数: プライベートリポジトリの解析を利用したいユーザ数に合わせて購入が必要
  • プライベートリポジトリ数: 無制限
  • パブリックリポジトリ: 無制限
  • プラン: プライベートリポジトリへアクセスするユーザ1人につき1,500円/月(税込み)

料金プラン改定の背景

GitHubの料金プランは、2016年5月にリポジトリ数ベースからユーザ数ベースに変更されており、開発者の人数と比べたときに、多くのリポジトリを運用している開発チームが多数存在します。一方で、SideCIではリポジトリ数に応じて利用料金が決まる、リポジトリ数ベースの料金プランをこれまで採用してきました。この、利用料金の計算に対するギャップがSideCIをご利用いただく際の障壁の一つとなっていると私たちは考えており、今回の料金プラン変更へとつながりました。

また、SideCIの提供において必要となる計算機リソースは、Pull RequestのOpenされる数や更新される数と比例します。これは、その開発チームの持つリポジトリの数ではなく、所属している開発者の数と強く関連しています。そのため、リポジトリ数ではなく、それにアクセスするユーザ数に応じた料金プランがSideCIのサービス提供に対して適正であると考えています。

新プランへの移行のお願い

旧プランでSideCIをご利用いただいているみなさまにも、新プランへの移行をお願いします。

  • 新しい料金プランでは、プライベートリポジトリを無制限に登録可能ですので、今までSideCIを使えていなかったリポジトリでも、SideCIを気軽に導入いただけるようになります
  • GitHub上のOrganizationへの登録ユーザ数ではなく、実際にSideCIを利用するユーザのみを課金対象としています

こうしたメリットがありますので、お手数ではございますが、現在ご契約いただいているお客様も、新プランに移行いただけましたら幸いです。

その他の新機能

また、SideCIでは5月1日より、次の機能の提供を開始します。

  • SideCIのWebサイトから領収書を確認、印刷できるようになります
  • 料金プラン変更時(ユーザの増減時)に日割り計算を行ないます

従来、請求書及び領収書はメールで送信されておりました。このメールの宛先変更や、再送付はお問い合わせいただく事が必要でしたが、今後は上記のように、Web UIから簡単に確認が行えるようになります。

新料金プランへの移行について

現在、SideCIで有料プランをご利用いただいているお客様については、当面は旧プランでのご利用を継続していただけます。5月1日以降は、旧プランへの変更および旧プランの間でのアップグレード、ダウングレードや新規の利用開始ができなくなりますので、ご了承ください。

新プランに移行される際には、SideCIのWebサイトから変更が可能です。(これまでPayPalを利用して利用料金のお支払いをされていた場合には、改めてクレジットカードの登録が必要になりますので、ご了承ください。)

画面上での操作について

次のような手順でプランを変更いただけます。(5月1日以降に以下のような画面が適用されます。)

オーガニゼーション設定ページに遷移していただくと、契約中のプランが次のように表示されます。 f:id:sideci:20180425174248p:plain

上記の「Upgrade」ボタンを押して、プラン変更を進めて下さい。クレジットカードが未登録の場合にはクレジットカード登録画面が現れます。

なお、領収書の確認、印刷などは次のような画面で行う事ができます。

f:id:sideci:20180425174843p:plain Invoice 一覧画面

f:id:sideci:20180425174858p:plain Invoice

よくある質問

旧プランを新しく契約することはできますか?

5月1日以降、旧プランを契約することはできなくなります。ご了承ください。

旧プランを現在契約中です。このプランはいつまで利用可能ですか?

現在(2018-04-25)のところ、旧プランの廃止時期は決まっておりません。
旧プランの廃止の際には、その3ヶ月前を目処に告知いたします。

旧プラン体系での契約の変更または新規の契約はできますか?

5月1日以降、旧プランでの契約の変更や新しく契約することはできません。新プランへの移行をお願いします。

ユーザ数はどうやって計算されますか?

まず、SideCIのプラン設定画面で、お客様に必要なユーザ数をご指定いただきます。
お客様の指定したユーザ数を上限として、SideCIの設定画面から、プライベートリポジトリへのアクセスを許可したいユーザに対して権限を付与できます。
プライベートレポジトリへアクセスできるSideCIユーザの数は、お客様によるSideCI上での操作以外によって、変更されることはありません。つまり、新しくユーザがサインアップしたとしても、SideCIの利用料金は自動では変更されません。

ユーザ数の増減はどう処理されますか?

日割りの計算が行われます。月の途中でいつでもユーザ数を増やすことや減らすことが可能です。

RuboCopコミッタ、Pockeが語るBatsov像とアドバイス -SideCI技術顧問就任記念インタビュー

SideCIでは、2018年4月より、RuboCopコミッタのPockeこと、桑原 仁雄氏を技術顧問に迎えることになりました!Pocke氏は、2015年9月にSideCIに入社して以来、SideCIが提供する解析器の拡充などに携わってきました。この4月よりクックパッド株式会社に転職し、同時にSideCIの技術顧問に就任しました。SideCIではこれを記念し、Pocke氏にインタビューを行いました。和やかな雰囲気の中、RuboCopから、Batsov氏のこと、そして未来のコミッタたちへのアドバイスなどを、ざっくばらんに話してくれました。ぜひご覧ください!

目次

f:id:sideci:20180308172024j:plain

RuboCop自体の話

- まずは、基本的な質問から始めましょう。RuboCopとは何でしょう?

RuboCopは一番括りの狭い言葉でいうとRubyの静的解析器なんです。それは何故かというと、静的解析器の中にスタイルチェッカーとLinterあとその他諸々がいっしょくたになったものだからです。その他諸々って言うのは、例えばメトリクスを測るようなものとかです。 特徴的なところでは、RuboCopのReadmeの一番最初に映画のロボコップのセリフが引用されているんです。「Role models are important.」

f:id:sideci:20180417115626p:plain 出典:http://rubocop.readthedocs.io/en/latest/

確かRuby Style Guideの方にも同じセリフが引用されているはずです。

RuboCopに関わり始めてからコミッタになるまで

-どういう経緯でRuboCopに関わるようになったんでしたっけ?

2015年の秋にSideCIに入ってからRuboCopに関わるようになりました。2016年に初めてプルリクを送ったんです。最初は割とどうでもいいプルリクを投げていたんですけど、そのうちRuboCopって結構バグがあって面白いって気がついて、ひたすらバグを直していた気がします。

- RuboCopでは主にどういう仕事をしていますか?

地味にバグを直していますね。

f:id:sideci:20180417115921p:plain Fix Style/RedundantReturn cop for empty if body #3607”(https://github.com/bbatsov/rubocop/pull/3607)

例えば、このPRでは、Rubyではifの中身が空のことがあるんですけど、そうするとRuboCopがクラッシュするっていう問題があって、それを直すものです。

他には、ちょっと前にRSpecをtest-queueで速くするっていうのをやりました。 https://github.com/bbatsov/rubocop/pull/4544

当時CIがめちゃくちゃ遅くて辛かったんですけど、結構速くできたので良かったですね。大体2〜3倍くらいは速くなりました。

-コミッタになったときの経緯とかは、なにかきっかけがあるんですか?

特にきっかけは無かったと思います。ある日突然Invitationが届いていたみたいな感じで。READMEのヒストリでPockeと検索すると出てくるのが、8ヶ月前ですね。 f:id:sideci:20180417120641p:plain なにか特別なことをやっていたわけでもありませんが、でも一番プルリクを送っていた時期だとは思います。

- コミッタになる前となった後で関わりかたになにか変わったところはありますか?

あまりないです。外に向けてコミッタやっているってことを言えるようになったっというのと、あとはIssueにラベルをつけられるようになったのが一番の違いです。

前に「5個ぐらい同じIssueが立っている」みたいなことがあって、それは辛かったですね。もちろん、一瞬でCloseできるので大きな問題ではないのですが。コミッタになったことで、Closeとかラベル付けとかできるようになってしまって、自分の問題になってしまった。ちょっと検索してみて欲しいって気持ちはあります。

Batsovの話

-Batsovはどんな人ですか?

ヨーロッパ圏でVPoEやってる方で、確か人材系の会社だった気がします。会ったことはないんですが、去年はEuRukoでトークしてましたね。今は積極的に自分でRuboCopのコードを書くということはしていなくて、でも別にRuboCopに興味を失ったということでもないみたいでレビューはがんがん来ます。プルリク等を見ていても反応は早い方で、帰ってこないときは旅行中とかそのような時だけ。そうでないときはこまめに返ってきてます。 全体的にマメで親切な人だという印象はありますね。レビューしてても、なにか言ったら、同じことを同じコードについて全部繰り返して書くとか。忘れなくて良いです。プルリクもほとんどマージしていて、そういう意味でも、寛容というか、全て受け入れてるなって気がしますね。大きすぎるとか、説明がないとか、そういうPRはどうしようもないので、pingを打って返ってこなかったらCloseするとかしますけど。

-RuboCopの名前とかさっきのREADMEの引用とか、ロボコップからの借用が多いような思いますが、誰の趣味なんでしょう?

Batsovの趣味だと思います。さっきの「Roll models are important.」はRubyStyleGuideにもありますし。あれもBatsovがやった仕事です。 面白かったのだと、RuboCopのルールのグループが分かれていますが、例えばStyleとかLintとか、あのグループのことを昔はcop_typeと言っていたんですけど、ある日突然departmentに変わったんですよ。 <<PR: Replace cop_type with department https://github.com/bbatsov/rubocop/pull/3842>>

これは、コップ(警察官)の部署という意味らしいんです。コミッショナー(所長)の下にteamがいたり、警察関連の言葉が用いられているんです。他にも、forceとかoffenceとか。forceってプログラミング用語とは関係がなくて、変数のトレースとかするものをさしています。構造を覚えるのが大変ですね。 (出典:2016/12/30 https://github.com/bbatsov/rubocop/pull/3837#issuecomment-269750599  この中でBatsovは” And I think we've strayed from our fun police references! cop_type? A cop should have a department, not a type! :-)”と述べている。

RuboCopの開発に参加するには

-RuboCop自体のコードレビューというのはどんな形で運用されているのですか?実際にレビューする人される人って側でのコメントを聞かせてください。

特に決められたレビュアーとかはいなくて、誰かが見るみたいな運用をしています。ただ、マージは基本的にBatsovがすることにはなっています。で、結構コミッタじゃない人もレビューしていて、活発な感じで動いています。

あんまり厳格なルールはないのですが、PRのテンプレートがリポジトリに入っているので、できるだけそれに沿って欲しいというところぐらいですね。Issueを上げてからPRを作る、みたいな作法のプロジェクトもありますが、RuboCopはそうではない。テストは追加して欲しいですけど。

レビューで気にするところは結構人によって異なっていて、結構コーディングスタイル的なところを突っ込む人もいます。名前とか英語とかも見られますね。私も、最初のころは全然英語ができなくって、Offenceのメッセージに「その英語おかしいよね」とか言われるのは結構ありました。

-新人コントリビュータへのアドバイスみたいなものはありますか?

私は、正しいRubyのコードで走らせた時にクラッシュしないかどうか、割と重視してみています。なので、一回作ったものをなるべく多くのコードで試した方がいいっていうのがアドバイスですね。私がきちんとやるときには、ruby/spec(github.com/ruby/spec)や、GitLab(github.com/gitlabhq/gitlabhq)とかの大きなRubyのプロジェクトで試すようにしていて、クラッシュしないかとある程度正しいオフェンスが出てるかを見るようにしています。

-「こういうのが手をつけやすそう」みたいなものはありますか?

RuboCopって、デフォルトの設定じゃないと簡単にクラッシュするので、ちょっと変わった設定をすると意外と簡単に問題が見つかります。RuboCop自体はRuboCopでチェックしてるんですが、ほぼデフォルトの設定になっていて、だからデフォルトの設定だとさすがになかなか問題は見つからない。

あとは、さっきのruby/specで、そこには結構面白いコードがたくさん入っているので、RuboCopにそこのコードを読ませると簡単にRuboCopが壊れます。

他にはこういうのがあります(#4910)。

f:id:sideci:20180417121108p:plain https://github.com/bbatsov/rubocop/issues/4910

これは「Documentationに例が欲しい」Issueを立てたら、結構みんなやってくれて。コメント足すだけなんで、簡単にできるといえば出来るんですけど。これがないと、テストケースを見ないと、そのコップが何をするかわからないという状況だったので。これなんか、手が付けやすかったんじゃないかと思います。

Issueはもうずっとつまっていて、まあ300件ぐらいオープンなものがあるんですけど、割とずっと積もっているままになっているのが多いです。なので丁寧に見ていけば、なにかやりやすいものがあるかもしれませんね。

- RuboCopの開発チームでこういうことをやっていきたいみたいなテーマはあったりしますか?

最近ですと「bump to 1.0」っていうIssueがあって、これ本当にスタートさせるのかな、と思ったりしています。

f:id:sideci:20180417121430p:plain bump to 1.0 のスクリーンショット

今は0.53.0なんですが、Batsovのコメントによると、まだ1.0にはしないって言ったりとかしている。これも品質が低いとしか言っていないのでなにをやっていきたいのかはっきりとはわからないんですが、まだまだ壊したいんじゃないですかね。そろそろ1.0を出して、壊れたら2.0を出すようにするとかしてもらいたいんですけど……

個人的に今やりたいのはオートコレクトですね。「ruby/specを入力にして、RuboCopでオートコレクトして、その結果がテストを通るかどうか」っていうのをやってみたいですね。現状はruby/specを入力しても全然ちゃんと動かないので。

後はちょっと使いにくいところがあって、「ソースコードの一部だけを選択的にオートコレクトしたい」みたいな要望があるんですけど、そういうのをサポートしたいですね。

- RuboCopの開発者って多分日本にも何人かいると思うんですけど、その人たちと何かオフラインで会って話したりしますか?

割と会うことがあります。最近プルリク投げているkoicさん (github.com/koic)、結構よくあっていて、RuboCopの話をすることがあります。懇親会とかが多いんですが、去年のRubyKaigiの時は、こういうの作ろうと思ってるんだけど、どう?みたいな相談をされて、一緒に話しながらプルリクを見ていったことがありました。

RuboCop利用者へ

-どういう風にRuboCopを使ってもらいたいみたいとか、こうやって風に使ってみたらいいよみたいとか、アドバイスはありますか?

RuboCopに期待しすぎないで欲しいっていうのはあります。銀の弾丸だと思っている人がいて。で、とりあえず入れたらコードがよくなるって思っている人がいると思うんですけど、そうではなくて、RuboCopにどうさせたいか、っていう目的がないと、ちゃんと使いづらいっていうのがあると思います。

例えば、RuboCopでコーディングスタイルを統一したいという目的があるんだったら、それ用のコーディングスタイルを作るとこから始めるか、RuboCopのコーディングをそのまま受け入れるみたいな選択が必要になります。なので、入れただけだと、警告が大量に出て失敗するっていう状況だけになるかなと思います。

RuboCopを使って何をやりたいのかというのをまず考えて、その上でちゃんと設定しないといけない。

-具体的には、それぞれどうやって設定を詰めて行ったらいいのでしょうか?

WEB+DB PRESSに「Rubyドキドキ探検隊」という連載を2017年からしているんですが、Vol.102で解説を書いたので、読んでいただければと(笑)。

ざっくりというなら、まずスタイルの方は、ひたすら頑張って直していくしかありません。.rubocop_todo.yml (http://rubocop.readthedocs.io/en/stable/configuration/#automatically-generated-configuration) っていう仕組みがあるので、それを使えば良いと思います。 これは、一回、今のソースコードで出る警告をToDoとして書き出した上で、警告は出ないようにするという仕組みで、まずはじめに出た警告を一旦見なかったことにして、少しずつ自分たちのルールを作って行きながら、適用をしていくということができる。結局その作業をしなければいけないのでいけないので、どこかで自力で頑張らなきゃいけないということになります。

バグを見つけるというのはもっと簡単で、単にDisabledByDefaultってオプションがあるので、一旦全部バッて無効にしてバグを見つける系のLint系のルールだけを有効に設定するのがいいです。 ruby/specの設定がちょうどこの使い方なので、参考になるかもしれません。

- 逆にRuboCop利用者にお願いしたい事はありますか?重複してバグを報告しない、以外で(笑)

バグは出来れば報告してほしいですね。日本人向けのことを言えば、rubocop-jp(github.com/rubocop-jp/issues)というorganizationを作ったので、そちらにバグを報告していただいても良いです。 これはvim-jp(github.com/vim-jp)を参考にしていて、rubocop-jpに上がってきたものを英語化して本家に投げるところまでできれば、理想的なんじゃないかと思います。

私がそもそも英語を書きたくないっていう思いがあって、でも英語でやらないと本家のIssueにあげられないじゃないですか。で、そうすると自分の中だけに溜まっていくIssueっていうのが大量に増えてきて、それを管理するのが嫌になってきたので、rubocop-jpを作ったというのも側面にあります。

SideCIについて

-SideCIでRuboCopを使うと便利だと思うんですが、その辺りを少し話していただけますか?

現状で言えば、割とLint系ってどうしてもfalse positiveが出るものだと思うんですけど、SideCIで動かしていると、そういうものでも割と気軽に有効にしていけるっていうのは、あると思います。

SideCIがない場合には、ローカルで動かすかCIの中で実行するかしかないんですが、ローカルでいちいち実行するのは面倒なのでCIでやろうかっていう話になると思います。ところが、それだと1か0かみたいな所があって、RuboCopがコケるかコケないかの二つの状態しか持てない。これは、不便なことが結構あるんじゃないかなと思います。SideCIがあると、RuboCopが出した指摘をCloseするかしないか選択できるので、「この警告は許容しておく」っていう状態が出来るので柔軟性が高くて、使いやすい状態になると思います。

(編集者注:SideCIではRuboCopなどのLINTツールに検出された問題を修正するのか、それとも今回は対応せずに見送るのかを、個々に設定することができます。実際に検出された問題を確認しながら、それぞれを修正するかしないか決められるので、より積極的にツールの検査を有効にすることができます。)

あとは今は実装できていない未来の話をすると、オートコレクトももっと上手くできると思っていて、自動で修正したいんだけどどう修正したら良いのかは自明ではなかったりするときに、インタラクティブに修正していけるようなUIがあると良いのかなと思います。

f:id:sideci:20180308172951j:plain

インタビューの最後には、SideCIのCTOの松本とPocke氏で記念撮影をしました。 Pockeさん、これからもどうぞよろしくお願いします!