superclass_delegating_accessor rewritten to serve as the base for many other kinds of accessors (step 1 of unification)

2 files changed, 27 insertions, 87 deletions

diff --git a/activesupport/lib/active_support/core_ext/class/delegating_attributes.rb b/activesupport/lib/active_support/core_ext/class/delegating_attributes.rb
index 72e0eefb0a..19382abb76 100644
--- a/activesupport/lib/active_support/core_ext/class/delegating_attributes.rb
+++ b/activesupport/lib/active_support/core_ext/class/delegating_attributes.rb
@@ -1,67 +1,41 @@
 require 'active_support/core_ext/object/blank'
 require 'active_support/core_ext/array/extract_options'
+require 'active_support/core_ext/object/metaclass'
 
 class Class
-  def superclass_delegating_reader(*names)
-    class_to_stop_searching_on = superclass.name.blank? ? "Object" : superclass.name
-    options = names.extract_options!
+  def superclass_delegating_accessor(name, options = {})
+    # Create private _name and _name= methods that can still be used if the public
+    # methods are overridden. This allows
+    _superclass_delegating_accessor("_#{name}")
 
-    names.each do |name|
-      # def self.only_reader
-      #   if defined?(@only_reader)
-      #     @only_reader
-      #   elsif superclass < Object && superclass.respond_to?(:only_reader)
-      #     superclass.only_reader
-      #   end
-      # end
-      class_eval <<-EOS, __FILE__, __LINE__ + 1
-        def self.#{name}
-          if defined?(@#{name})
-            @#{name}
-          elsif superclass < #{class_to_stop_searching_on} && superclass.respond_to?(:#{name})
-            superclass.#{name}
-          end
-        end
-      EOS
+    # Generate the public methods name, name=, and name?
+    # These methods dispatch to the private _name, and _name= methods, making them
+    # overridable
+    metaclass.send(:define_method, name) { send("_#{name}") }
+    metaclass.send(:define_method, "#{name}?") { !!send("_#{name}") }
+    metaclass.send(:define_method, "#{name}=") { |value| send("_#{name}=", value) }
 
-      unless options[:instance_reader] == false
-        class_eval <<-EOS, __FILE__, __LINE__ + 1
-          def #{name}                                  # def only_reader
-            self.class.#{name}                         #   self.class.only_reader
-          end                                          # end
-          def self.#{name}?                            # def self.only_reader?
-            !!#{name}                                  #   !!only_reader
-          end                                          # end
-          def #{name}?                                 # def only_reader?
-            !!#{name}                                  #   !!only_reader
-          end                                          # end
-        EOS
-      end
-    end
+    # If an instance_reader is needed, generate methods for name and name= on the
+    # class itself, so instances will be able to see them
+    define_method(name) { send("_#{name}") } if options[:instance_reader] != false
+    define_method("#{name}?") { !!send("#{name}") } if options[:instance_reader] != false
   end
 
-  def superclass_delegating_writer(*names, &block)
-    options = names.extract_options!
+private
 
-    names.each do |name|
-      class_eval <<-EOS, __FILE__, __LINE__ + 1
-        def self.#{name}=(value)     # def self.property=(value)
-          @#{name} = value           #   @property = value
-        end                          # end
-      EOS
+  # Take the object being set and store it in a method. This gives us automatic
+  # inheritance behavior, without having to store the object in an instance
+  # variable and look up the superclass chain manually.
+  def _stash_object_in_method(object, method, instance_reader = true)
+    metaclass.send(:define_method, method) { object }
+    define_method(method) { object } if instance_reader
+  end
 
-      self.send(:"#{name}=", yield) if block_given?
+  def _superclass_delegating_accessor(name, options = {})
+    metaclass.send(:define_method, "#{name}=") do |value|
+      _stash_object_in_method(value, name, options[:instance_reader] != false)
     end
+    self.send("#{name}=", nil)
   end
 
-  # These class attributes behave something like the class
-  # inheritable accessors.  But instead of copying the hash over at
-  # the time the subclass is first defined, the accessors simply
-  # delegate to their superclass unless they have been given a 
-  # specific value.  This stops the strange situation where values 
-  # set after class definition don't get applied to subclasses.
-  def superclass_delegating_accessor(*names, &block)
-    superclass_delegating_reader(*names)
-    superclass_delegating_writer(*names, &block)
-  end
 end
diff --git a/activesupport/test/core_ext/class/delegating_attributes_test.rb b/activesupport/test/core_ext/class/delegating_attributes_test.rb
index beb55ba17e..011068ab74 100644
--- a/activesupport/test/core_ext/class/delegating_attributes_test.rb
+++ b/activesupport/test/core_ext/class/delegating_attributes_test.rb
@@ -21,27 +21,6 @@ class DelegatingAttributesTest < Test::Unit::TestCase
     @single_class = Class.new(Object)
   end
 
-  def test_simple_reader_declaration
-    single_class.superclass_delegating_reader   :only_reader
-    # The class and instance should have an accessor, but there
-    # should be no mutator
-    assert single_class.respond_to?(:only_reader)
-    assert single_class.respond_to?(:only_reader?)
-    assert single_class.public_instance_methods.map(&:to_s).include?("only_reader")
-    assert single_class.public_instance_methods.map(&:to_s).include?("only_reader?")
-    assert !single_class.respond_to?(:only_reader=)
-  end
-
-  def test_simple_writer_declaration
-    single_class.superclass_delegating_writer   :only_writer
-    # The class should have a mutator, the instances shouldn't
-    # neither should have an accessor
-    assert single_class.respond_to?(:only_writer=)
-    assert !single_class.public_instance_methods.include?("only_writer=")
-    assert !single_class.public_instance_methods.include?("only_writer")
-    assert !single_class.respond_to?(:only_writer)
-  end
-
   def test_simple_accessor_declaration
     single_class.superclass_delegating_accessor :both
     # Class should have accessor and mutator
@@ -74,19 +53,6 @@ class DelegatingAttributesTest < Test::Unit::TestCase
     assert_equal false, single_class.both?
   end
 
-  def test_working_with_accessors
-    single_class.superclass_delegating_reader   :only_reader
-    single_class.instance_variable_set("@only_reader", "reading only")
-    assert_equal "reading only", single_class.only_reader
-    assert_equal "reading only", single_class.new.only_reader
-  end
-
-  def test_working_with_simple_mutators
-    single_class.superclass_delegating_writer   :only_writer
-    single_class.only_writer="written"
-    assert_equal "written", single_class.instance_variable_get("@only_writer")
-  end
-
   def test_child_class_delegates_to_parent_but_can_be_overridden
     parent = Class.new
     parent.superclass_delegating_accessor :both