ActiveDecoratorにプルリクを投げるまでにやったこと
怪しい挙動を見つけたきっかけ
テストコードで以下のようなことをしてました。
Rails.application.eager_load!
testでもproductionと同じ環境になるように事前に読み込んでいたのですが、テストを個別に実行するときにも全ファイルを読み込んでしまうため消したいと思っていました。
試しに消してCIを回してみると、一部のテストがfailした。
どんな問題が起きたか?
再現するコード例は以下の通り。
form = Form::User.new ActiveDecorator::Decorator.instance.decorate(form) form.is_a?(UserDecorator) #=> true
Form::UserDecorator
を定義してないときに、なぜか UserDecorator
が使われてしまう。
コードを読んで発生箇所を突き止める
ActiveDecorator のコード内で Object.const_get
を使っている箇所が UserDecorator
を返していた。
そして、モジュールの自動読み込みに関する処理は ActiveSupport に定義されている。
ここまで調べて、自動読み込み周りはどう直すべきかよく分からなかった。
OSSパッチ会で松田さんに聞く
ちょうど問題を見つけた2日後にパッチ会があったので、直接聞いてみることにした。
a_matsuda さんに聞いてみたところ、以下のことが分かった。
- 最近、そのあたりのコードを変えた
- v1.2.0、v1.3.0(最新)で挙動を確認してみて欲しい
- 意図した挙動ではない(バグっぽい)
動作確認してみると、 v1.2.0 では問題が起きず v1.3.0 で問題が起きること が判明した!
再現するテストケースを書く
v1.2.0 では問題ない事が分かったら、すぐ amatsuda/active_decorator@73dcdd6 が原因だと分かった。
問題を再現させるテストケースを書き、意図通りに失敗させて、 73dcdd6 をリバートしてテストが通ることを確認した。
英語のコミットログを書く
自分が最初に書いた英語のコミットログがこれ。
Fix a wrong decoration for nested classes There is a problem that unintended decorators are used for nested classes. This commit reverts 73dcdd67ae146ee9e76b1da9ae9e9dbd55529193 to fix the problem.
英語が苦手なので、OSSパッチ会に参加していた @yahonda さんにレビューしてもらいました。フィードバックは以下の通り。
wrong
やunintended
が何か分からない- actual と expected を書いた方が良い
- 細かいけど
a wrong decoration
の a は要らない unintended
よりunexpected
の方が良い
レビュー頂いて気づいたけど、英語の問題の前にコミットログの内容が足りていなかった...。
直したコミットログ
どういう問題が起きていて、何を期待しているかが分かるようにコミットログを直してプルリクを作成した。
Fix wrong decoration for nested classes ActiveDecorator decorates incorrectly for the nested class that do not define decorator. comic = Foo::Comic.new ActiveDecorator::Decorator.instance.decorate(comic) comic.is_a?(ComicDecorator) #=> true If `Foo::ComicDecorator` is not defined, it should not use `ComicDecorator` instead of `Foo::ComicDecorator`. The cause of the problem is 73dcdd6, so we will revert it.
まとめ
OSSパッチ会のおかげでプルリク作るところまで出来た。ありがたい。