Initial work on DeclarationList and cleaning up AttributeList

lib/factory_girl.rb

@@ -1,3 +1,5 @@
+require "active_support/core_ext/module/delegation"
+
 require 'factory_girl/proxy'
 require 'factory_girl/proxy/build'
 require 'factory_girl/proxy/create'
@@ -11,6 +13,7 @@ require 'factory_girl/attribute/dynamic'
 require 'factory_girl/attribute/association'
 require 'factory_girl/attribute/sequence'
 require 'factory_girl/callback'
+require 'factory_girl/declaration_list'
 require 'factory_girl/declaration'
 require 'factory_girl/declaration/static'
 require 'factory_girl/declaration/dynamic'

lib/factory_girl/attribute_list.rb

@@ -2,14 +2,12 @@ module FactoryGirl
   class AttributeList
     include Enumerable
 
-    attr_reader :callbacks, :declarations
-
     def initialize(name = nil)
       @name         = name
       @attributes   = {}
-      @declarations = []
-      @callbacks    = []
+      @declarations = DeclarationList.new
       @overridable  = false
+      @compiled     = false
     end
 
     def declare_attribute(declaration)
@@ -18,24 +16,21 @@ module FactoryGirl
     end
 
     def define_attribute(attribute)
-      if attribute.respond_to?(:factory) && attribute.factory == @name
-        raise AssociationDefinitionError, "Self-referencing association '#{attribute.name}' in '#{attribute.factory}'"
-      end
-
+      ensure_attribute_not_self_referencing! attribute
       ensure_attribute_not_defined! attribute
-      add_attribute attribute
-    end
 
-    def add_callback(callback)
-      @callbacks << callback
+      add_attribute attribute
     end
 
     def each(&block)
       flattened_attributes.each(&block)
     end
 
-    def apply_attributes(attributes_to_apply)
-      attributes_to_apply.callbacks.reverse.each { |callback| prepend_callback(callback) }
+    def ensure_compiled
+      compile unless @compiled
+    end
+
+    def apply_attribute_list(attributes_to_apply)
       new_attributes = []
 
       attributes_to_apply.each do |attribute|
@@ -53,19 +48,19 @@ module FactoryGirl
     end
 
     def overridable
+      @compiled = false
       @overridable = true
     end
 
-    def overridable?
-      @overridable
-    end
-
-    def size
-      to_a.size
-    end
-
     private
 
+    def compile
+      @declarations.to_attributes.each do |attribute|
+        define_attribute(attribute)
+      end
+      @compiled = true
+    end
+
     def add_attribute(attribute)
       delete_attribute(attribute.name) if overridable?
 
@@ -74,10 +69,6 @@ module FactoryGirl
       attribute
     end
 
-    def prepend_callback(callback)
-      @callbacks.unshift(callback)
-    end
-
     def prepend_attributes(new_attributes)
       new_attributes.group_by {|attr| attr.priority }.each do |priority, attributes|
         @attributes[priority] ||= []
@@ -98,6 +89,12 @@ module FactoryGirl
       end
     end
 
+    def ensure_attribute_not_self_referencing!(attribute)
+      if attribute.respond_to?(:factory) && attribute.factory == @name
+        raise AssociationDefinitionError, "Self-referencing association '#{attribute.name}' in '#{attribute.factory}'"
+      end
+    end
+
     def attribute_defined?(attribute_name)
       !!find_attribute(attribute_name)
     end
@@ -115,5 +112,9 @@ module FactoryGirl
         end
       end
     end
+
+    def overridable?
+      @overridable
+    end
   end
 end

lib/factory_girl/declaration_list.rb

@@ -0,0 +1,15 @@
+module FactoryGirl
+  class DeclarationList
+    def initialize
+      @definitions = []
+    end
+
+    def to_attributes
+      @definitions.inject([]) {|result, definition| result += definition.to_attributes }
+    end
+
+    def method_missing(name, *args, &block)
+      @definitions.send(name, *args, &block)
+    end
+  end
+end

lib/factory_girl/factory.rb

@@ -1,5 +1,4 @@
 require "active_support/core_ext/hash/keys"
-require "active_support/core_ext/module/delegation"
 require "active_support/inflector"
 
 module FactoryGirl
@@ -16,16 +15,20 @@ module FactoryGirl
       @default_strategy = options[:default_strategy]
       @defined_traits   = []
       @attribute_list   = build_attribute_list
-      @compiled         = false
+      @callbacks = []
     end
 
-    delegate :overridable?, :declarations, :declare_attribute, :define_attribute, :add_callback, :to => :@attribute_list
+    delegate :declare_attribute, :to => :@attribute_list
 
     def factory_name
       $stderr.puts "DEPRECATION WARNING: factory.factory_name is deprecated; use factory.name instead."
       name
     end
 
+    def add_callback(callback)
+      @callbacks << callback
+    end
+
     def build_class #:nodoc:
       @build_class ||= class_name.to_s.camelize.constantize
     end
@@ -35,7 +38,6 @@ module FactoryGirl
     end
 
     def allow_overrides
-      @compiled = false
       @attribute_list.overridable
       self
     end
@@ -45,8 +47,6 @@ module FactoryGirl
     end
 
     def run(proxy_class, overrides, &block) #:nodoc:
-      ensure_compiled
-
       runner_options = {
         :attributes  => attributes,
         :callbacks   => callbacks,
@@ -111,8 +111,9 @@ module FactoryGirl
       @to_create_block = block
     end
 
-    def ensure_compiled
-      compile unless @compiled
+    def compile
+      parent.compile if parent
+      @attribute_list.ensure_compiled
     end
 
     protected
@@ -122,40 +123,25 @@ module FactoryGirl
     end
 
     def attributes
-      ensure_compiled
+      compile
       build_attribute_list.tap do |list|
-        @traits.reverse.map { |name| trait_by_name(name) }.each do |trait|
-          list.apply_attributes(trait.attributes)
+        traits.each do |trait|
+          list.apply_attribute_list(trait.attributes)
         end
 
-        list.apply_attributes(@attribute_list)
-        list.apply_attributes(parent.attributes) if parent
+        list.apply_attribute_list(@attribute_list)
+        list.apply_attribute_list(parent.attributes) if parent
       end
     end
 
+    def callbacks
+      [@callbacks].tap do |result|
+        result.unshift(*parent.callbacks) if parent
+      end.flatten
+    end
+
     private
 
-    def callbacks
-      attributes.callbacks
-    end
-
-    def compile
-      inherit_factory(parent) if parent
-
-      declarations.each do |declaration|
-        declaration.to_attributes.each do |attribute|
-          define_attribute(attribute)
-        end
-      end
-
-      @compiled = true
-    end
-
-    def inherit_factory(parent) #:nodoc:
-      parent.ensure_compiled
-      allow_overrides if parent.overridable?
-    end
-
     def assert_valid_options(options)
       options.assert_valid_keys(:class, :parent, :default_strategy, :aliases, :traits)
 
@@ -166,6 +152,10 @@ module FactoryGirl
       end
     end
 
+    def traits
+      @traits.reverse.map { |name| trait_by_name(name) }
+    end
+
     def trait_for(name)
       @defined_traits.detect {|trait| trait.name == name }
     end

lib/factory_girl/step_definitions.rb

@@ -95,7 +95,7 @@ end
 World(FactoryGirlStepHelpers)
 
 FactoryGirl.factories.each do |factory|
-  factory.ensure_compiled
+  factory.compile
   factory.human_names.each do |human_name|
     Given /^the following (?:#{human_name}|#{human_name.pluralize}) exists?:$/i do |table|
       table.hashes.each do |human_hash|

lib/factory_girl/syntax/default.rb

@@ -46,7 +46,7 @@ module FactoryGirl
           factory = FactoryGirl.factory_by_name(name).allow_overrides
           proxy = FactoryGirl::DefinitionProxy.new(factory)
           proxy.instance_eval(&block)
-          factory.ensure_compiled
+          factory.compile
         end
       end
     end

lib/factory_girl/trait.rb

@@ -11,24 +11,11 @@ module FactoryGirl
       proxy.instance_eval(&@block) if block_given?
     end
 
-    def declare_attribute(declaration)
-      @attribute_list.declare_attribute(declaration)
-      declaration
-    end
-
-    def add_callback(name, &block)
-      @attribute_list.add_callback(Callback.new(name, block))
-    end
+    delegate :declare_attribute, :to => :@attribute_list
 
     def attributes
-      AttributeList.new.tap do |list|
-        @attribute_list.declarations.each do |declaration|
-          declaration.to_attributes.each do |attribute|
-            list.define_attribute(attribute)
-          end
-        end
-        list.apply_attributes @attribute_list
-      end
+      @attribute_list.ensure_compiled
+      @attribute_list
     end
 
     def names

spec/factory_girl/attribute_list_spec.rb

@@ -60,19 +60,7 @@ describe FactoryGirl::AttributeList, "#define_attribute with a named attribute l
   end
 end
 
-describe FactoryGirl::AttributeList, "#add_callback" do
-  let(:proxy_class) { mock("klass") }
-  let(:proxy) { FactoryGirl::Proxy.new(proxy_class) }
-
-  it "allows for defining adding a callback" do
-    subject.add_callback(FactoryGirl::Callback.new(:after_create, lambda { "Called after_create" }))
-
-    subject.callbacks.first.name.should == :after_create
-    subject.callbacks.first.run(nil, nil).should == "Called after_create"
-  end
-end
-
-describe FactoryGirl::AttributeList, "#apply_attributes" do
+describe FactoryGirl::AttributeList, "#apply_attribute_list" do
   let(:full_name_attribute) { FactoryGirl::Attribute::Static.new(:full_name, "John Adams", false) }
   let(:city_attribute)      { FactoryGirl::Attribute::Static.new(:city, "Boston", false) }
   let(:email_attribute)     { FactoryGirl::Attribute::Dynamic.new(:email, false, lambda {|model| "#{model.full_name}@example.com" }) }
@@ -86,20 +74,20 @@ describe FactoryGirl::AttributeList, "#apply_attributes" do
 
   it "prepends applied attributes" do
     subject.define_attribute(full_name_attribute)
-    subject.apply_attributes(list(city_attribute))
+    subject.apply_attribute_list(list(city_attribute))
     subject.to_a.should == [city_attribute, full_name_attribute]
   end
 
   it "moves non-static attributes to the end of the list" do
     subject.define_attribute(full_name_attribute)
-    subject.apply_attributes(list(city_attribute, email_attribute))
+    subject.apply_attribute_list(list(city_attribute, email_attribute))
     subject.to_a.should == [city_attribute, full_name_attribute, email_attribute]
   end
 
   it "maintains order of non-static attributes" do
     subject.define_attribute(full_name_attribute)
     subject.define_attribute(login_attribute)
-    subject.apply_attributes(list(city_attribute, email_attribute))
+    subject.apply_attribute_list(list(city_attribute, email_attribute))
     subject.to_a.should == [city_attribute, full_name_attribute, email_attribute, login_attribute]
   end
 
@@ -107,7 +95,7 @@ describe FactoryGirl::AttributeList, "#apply_attributes" do
     subject.define_attribute(full_name_attribute)
     attribute_with_same_name = FactoryGirl::Attribute::Static.new(:full_name, "Benjamin Franklin", false)
 
-    subject.apply_attributes(list(attribute_with_same_name))
+    subject.apply_attribute_list(list(attribute_with_same_name))
     subject.to_a.should == [full_name_attribute]
   end
 
@@ -116,20 +104,20 @@ describe FactoryGirl::AttributeList, "#apply_attributes" do
 
     it "prepends applied attributes" do
       subject.define_attribute(full_name_attribute)
-      subject.apply_attributes(list(city_attribute))
+      subject.apply_attribute_list(list(city_attribute))
       subject.to_a.should == [city_attribute, full_name_attribute]
     end
 
     it "moves non-static attributes to the end of the list" do
       subject.define_attribute(full_name_attribute)
-      subject.apply_attributes(list(city_attribute, email_attribute))
+      subject.apply_attribute_list(list(city_attribute, email_attribute))
       subject.to_a.should == [city_attribute, full_name_attribute, email_attribute]
     end
 
     it "maintains order of non-static attributes" do
       subject.define_attribute(full_name_attribute)
       subject.define_attribute(login_attribute)
-      subject.apply_attributes(list(city_attribute, email_attribute))
+      subject.apply_attribute_list(list(city_attribute, email_attribute))
       subject.to_a.should == [city_attribute, full_name_attribute, email_attribute, login_attribute]
     end
 
@@ -137,7 +125,7 @@ describe FactoryGirl::AttributeList, "#apply_attributes" do
       subject.define_attribute(full_name_attribute)
       attribute_with_same_name = FactoryGirl::Attribute::Static.new(:full_name, "Benjamin Franklin", false)
 
-      subject.apply_attributes(list(attribute_with_same_name))
+      subject.apply_attribute_list(list(attribute_with_same_name))
       subject.to_a.should == [attribute_with_same_name]
     end
   end

spec/factory_girl/declaration_list_spec.rb

@@ -0,0 +1,17 @@
+require "spec_helper"
+
+describe FactoryGirl::DeclarationList, "#to_attributes" do
+  let(:static_attribute_1)  { stub("static attribute 1") }
+  let(:static_attribute_2)  { stub("static attribute 2") }
+  let(:dynamic_attribute_1) { stub("dynamic attribute 1") }
+  let(:static_declaration)  { stub("static declaration", :to_attributes => [static_attribute_1, static_attribute_2]) }
+  let(:dynamic_declaration) { stub("static declaration", :to_attributes => [dynamic_attribute_1]) }
+
+  it "returns all attributes by declaration order" do
+    subject << static_declaration
+    subject.to_attributes.should == [static_attribute_1, static_attribute_2]
+
+    subject << dynamic_declaration
+    subject.to_attributes.should == [static_attribute_1, static_attribute_2, dynamic_attribute_1]
+  end
+end

spec/factory_girl/factory_spec.rb

@@ -100,13 +100,13 @@ describe FactoryGirl::Factory do
 
   it "creates a new factory using the class of the parent" do
     child = FactoryGirl::Factory.new(:child, :parent => @factory.name)
-    child.ensure_compiled
+    child.compile
     child.build_class.should == @factory.build_class
   end
 
   it "creates a new factory while overriding the parent class" do
     child = FactoryGirl::Factory.new(:child, :class => String, :parent => @factory.name)
-    child.ensure_compiled
+    child.compile
     child.build_class.should == String
   end
 end
@@ -199,7 +199,7 @@ describe FactoryGirl::Factory do
 
   describe "defining a child factory without setting default strategy" do
     subject { FactoryGirl::Factory.new(:other_object, :parent => factory_with_stub_strategy.name) }
-    before  { subject.ensure_compiled }
+    before  { subject.compile }
 
     it "inherits default strategy from its parent" do
       subject.default_strategy.should == :stub
@@ -208,7 +208,7 @@ describe FactoryGirl::Factory do
 
   describe "defining a child factory with a default strategy" do
     subject { FactoryGirl::Factory.new(:other_object, :default_strategy => :build, :parent => factory_with_stub_strategy.name) }
-    before  { subject.ensure_compiled }
+    before  { subject.compile }
 
     it "overrides the default strategy from parent" do
       subject.default_strategy.should == :build