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.

1 files changed, 5 insertions, 5 deletions

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