diff options
author | Dmitriy Kiriyenko <dmitriy.kiriyenko@anahoret.com> | 2012-08-15 15:43:04 +0300 |
---|---|---|
committer | Dmitriy Kiriyenko <dmitriy.kiriyenko@anahoret.com> | 2012-12-20 11:06:45 +0200 |
commit | 4a9644a0d94a88896e1ebd3329b8f796fbd053d1 (patch) | |
tree | 40f5bef748e44220479a3ecd15dd17352eb0fdab /activesupport/test | |
parent | 01d3a36bfe5d56d85f8a36f2fe10ad96662b4530 (diff) | |
download | rails-4a9644a0d94a88896e1ebd3329b8f796fbd053d1.tar.gz rails-4a9644a0d94a88896e1ebd3329b8f796fbd053d1.tar.bz2 rails-4a9644a0d94a88896e1ebd3329b8f796fbd053d1.zip |
Prevent callback from being set twice.
When you add one callack in two separate `set_callback` calls - it is
only called once.
When you do it in one `set_callback` call - it is called twice.
This violates the principle of least astonishment for me. Duplicating
callback is usually an error. There is a correct and obvious way to do
anything without this "feature".
If you want to do
before_save :clear_balance, :calculate_tax, :clear_balance
or whatever, you should better do
before_save :carefully_calculate_tax
def carefully_calculate_tax
clear_balance
calculate_tax
clear_balance
end
And this even opens gates for some advanced refactorings, unlike the
first approach.
My assumptions are:
- Principle of least astonishment is violated, when callbacks are either
prevented from duplication, or not.
- Duplicating callbacks is usually an error. When it is intentional -
it's a smell of a bad design and can be approached without abusing
this "feature".
My suggestion is: do not allow duplicating callbacks in one callback
call, like it is not allowed in separate callbacks call.
Diffstat (limited to 'activesupport/test')
-rw-r--r-- | activesupport/test/callbacks_test.rb | 62 | ||||
-rw-r--r-- | activesupport/test/test_test.rb | 10 |
2 files changed, 62 insertions, 10 deletions
diff --git a/activesupport/test/callbacks_test.rb b/activesupport/test/callbacks_test.rb index 8810302f40..13f2e3cdaf 100644 --- a/activesupport/test/callbacks_test.rb +++ b/activesupport/test/callbacks_test.rb @@ -297,7 +297,7 @@ module CallbacksTest end end end - + class AroundPersonResult < MySuper attr_reader :result @@ -308,7 +308,7 @@ module CallbacksTest def tweedle_dum @result = yield end - + def tweedle_1 :tweedle_1 end @@ -316,7 +316,7 @@ module CallbacksTest def tweedle_2 :tweedle_2 end - + def save run_callbacks :save do :running @@ -410,7 +410,7 @@ module CallbacksTest ], around.history end end - + class AroundCallbackResultTest < ActiveSupport::TestCase def test_save_around around = AroundPersonResult.new @@ -607,6 +607,45 @@ module CallbacksTest end end + class OneTwoThreeSave + include ActiveSupport::Callbacks + + define_callbacks :save + + attr_accessor :record + + def initialize + @record = [] + end + + def save + run_callbacks :save do + @record << "yielded" + end + end + + def first + @record << "one" + end + + def second + @record << "two" + end + + def third + @record << "three" + end + end + + class DuplicatingCallbacks < OneTwoThreeSave + set_callback :save, :before, :first, :second + set_callback :save, :before, :first, :third + end + + class DuplicatingCallbacksInSameCall < OneTwoThreeSave + set_callback :save, :before, :first, :second, :first, :third + end + class UsingObjectTest < ActiveSupport::TestCase def test_before_object u = UsingObjectBefore.new @@ -709,5 +748,18 @@ module CallbacksTest end end end - + + class ExcludingDuplicatesCallbackTest < ActiveSupport::TestCase + def test_excludes_duplicates_in_separate_calls + model = DuplicatingCallbacks.new + model.save + assert_equal ["two", "one", "three", "yielded"], model.record + end + + def test_excludes_duplicates_in_one_call + model = DuplicatingCallbacksInSameCall.new + model.save + assert_equal ["two", "one", "three", "yielded"], model.record + end + end end diff --git a/activesupport/test/test_test.rb b/activesupport/test/test_test.rb index 9516556844..d6821135ea 100644 --- a/activesupport/test/test_test.rb +++ b/activesupport/test/test_test.rb @@ -122,12 +122,12 @@ end # Setup and teardown callbacks. class SetupAndTeardownTest < ActiveSupport::TestCase setup :reset_callback_record, :foo - teardown :foo, :sentinel, :foo + teardown :foo, :sentinel def test_inherited_setup_callbacks assert_equal [:reset_callback_record, :foo], self.class._setup_callbacks.map(&:raw_filter) assert_equal [:foo], @called_back - assert_equal [:foo, :sentinel, :foo], self.class._teardown_callbacks.map(&:raw_filter) + assert_equal [:foo, :sentinel], self.class._teardown_callbacks.map(&:raw_filter) end def setup @@ -147,7 +147,7 @@ class SetupAndTeardownTest < ActiveSupport::TestCase end def sentinel - assert_equal [:foo, :foo], @called_back + assert_equal [:foo], @called_back end end @@ -159,7 +159,7 @@ class SubclassSetupAndTeardownTest < SetupAndTeardownTest def test_inherited_setup_callbacks assert_equal [:reset_callback_record, :foo, :bar], self.class._setup_callbacks.map(&:raw_filter) assert_equal [:foo, :bar], @called_back - assert_equal [:foo, :sentinel, :foo, :bar], self.class._teardown_callbacks.map(&:raw_filter) + assert_equal [:foo, :sentinel, :bar], self.class._teardown_callbacks.map(&:raw_filter) end protected @@ -168,7 +168,7 @@ class SubclassSetupAndTeardownTest < SetupAndTeardownTest end def sentinel - assert_equal [:foo, :bar, :bar, :foo], @called_back + assert_equal [:foo, :bar, :bar], @called_back end end |