Wrap up DeclarationList

DeclarationList knows how to generate an attribute
list, which never really made sense outside of being generated from
declarations. Now, the declaration list builds a list of attributes
which is combined in Factory#attributes with attributes from traits and
its parents.

lib/factory_girl/attribute_list.rb

@@ -2,19 +2,9 @@ module FactoryGirl
   class AttributeList
     include Enumerable
 
-    attr_reader :declarations
-
     def initialize(name = nil)
       @name       = name
       @attributes = {}
-      @declarations = DeclarationList.new
-      @overridable  = false
-      @compiled     = false
-    end
-
-    def declare_attribute(declaration)
-      @declarations << declaration
-      declaration
     end
 
     def define_attribute(attribute)
@@ -28,20 +18,11 @@ module FactoryGirl
       flattened_attributes.each(&block)
     end
 
-    def ensure_compiled
-      compile unless @compiled
-    end
-
-    def apply_attribute_list(attributes_to_apply)
+    def apply_attributes(attributes_to_apply)
       new_attributes = []
 
       attributes_to_apply.each do |attribute|
-        new_attribute = if !overridable? && defined_attribute = find_attribute(attribute.name)
-          defined_attribute
-        else
-          attribute
-        end
-
+        new_attribute = find_attribute(attribute.name) || attribute
         delete_attribute(attribute.name)
         new_attributes << new_attribute
       end
@@ -49,23 +30,9 @@ module FactoryGirl
       prepend_attributes new_attributes
     end
 
-    def overridable
-      @compiled = false
-      @overridable = true
-    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?
-
       @attributes[attribute.priority] ||= []
       @attributes[attribute.priority] << attribute
       attribute
@@ -86,7 +53,7 @@ module FactoryGirl
     end
 
     def ensure_attribute_not_defined!(attribute)
-      if !overridable? && attribute_defined?(attribute.name)
+      if attribute_defined?(attribute.name)
         raise AttributeDefinitionError, "Attribute already defined: #{attribute.name}"
       end
     end
@@ -114,9 +81,5 @@ module FactoryGirl
         end
       end
     end
-
-    def overridable?
-      @overridable
-    end
   end
 end

lib/factory_girl/declaration_list.rb

@@ -1,15 +1,48 @@
 module FactoryGirl
   class DeclarationList
-    def initialize
+    include Enumerable
+
+    def initialize(name = nil)
       @declarations = []
+      @name         = name
+      @overridable  = false
+    end
+
+    def declare_attribute(declaration)
+      delete_declaration(declaration) if overridable?
+
+      @declarations << declaration
+      declaration
+    end
+
+    def overridable
+      @overridable = true
+    end
+
+    def attribute_list
+      AttributeList.new(@name).tap do |list|
+        to_attributes.each do |attribute|
+          list.define_attribute(attribute)
+        end
+      end
+    end
+
+    def each(&block)
+      @declarations.each(&block)
+    end
+
+    private
+
+    def delete_declaration(declaration)
+      @declarations.delete_if {|decl| decl.name == declaration.name }
     end
 
     def to_attributes
       @declarations.inject([]) {|result, declaration| result += declaration.to_attributes }
     end
 
-    def method_missing(name, *args, &block)
-      @declarations.send(name, *args, &block)
+    def overridable?
+      @overridable
     end
   end
 end

lib/factory_girl/definition.rb

@@ -1,16 +1,29 @@
 module FactoryGirl
   class Definition
-    attr_reader :callbacks, :defined_traits, :attribute_list
+    attr_reader :callbacks, :defined_traits, :declarations
 
     def initialize(name = nil)
-      @attribute_list = AttributeList.new(name)
+      @declarations   = DeclarationList.new(name)
       @callbacks      = []
       @defined_traits = []
       @to_create      = nil
       @traits         = []
     end
 
-    delegate :declare_attribute, :to => :attribute_list
+    delegate :declare_attribute, :to => :declarations
+
+    def attributes
+      @attributes ||= declarations.attribute_list
+    end
+
+    def compile
+      attributes
+    end
+
+    def overridable
+      declarations.overridable
+      self
+    end
 
     def traits
       @traits.reverse.map { |name| trait_by_name(name) }

lib/factory_girl/factory.rb

@@ -33,11 +33,6 @@ module FactoryGirl
       @default_strategy || parent.default_strategy || :create
     end
 
-    def allow_overrides
-      attribute_list.overridable
-      self
-    end
-
     def run(proxy_class, overrides, &block) #:nodoc:
       runner_options = {
         :attributes  => attributes,
@@ -92,7 +87,7 @@ module FactoryGirl
     def compile
       parent.defined_traits.each {|trait| define_trait(trait) }
       parent.compile
-      attribute_list.ensure_compiled
+      @definition.compile
     end
 
     protected
@@ -105,11 +100,11 @@ module FactoryGirl
       compile
       AttributeList.new(@name).tap do |list|
         traits.each do |trait|
-          list.apply_attribute_list(trait.attributes)
+          list.apply_attributes(trait.attributes)
         end
 
-        list.apply_attribute_list(attribute_list)
-        list.apply_attribute_list(parent.attributes)
+        list.apply_attributes(@definition.attributes)
+        list.apply_attributes(parent.attributes)
       end
     end
 
@@ -137,10 +132,6 @@ module FactoryGirl
       end
     end
 
-    def attribute_list
-      @definition.attribute_list
-    end
-
     class Runner
       def initialize(options = {})
         @attributes  = options[:attributes]

lib/factory_girl/null_factory.rb

@@ -6,14 +6,10 @@ module FactoryGirl
       @definition = Definition.new
     end
 
-    delegate :defined_traits, :callbacks, :to => :definition
+    delegate :defined_traits, :callbacks, :attributes, :to => :definition
 
     def compile; end
     def default_strategy; end
     def class_name; end
-
-    def attributes
-      AttributeList.new
-    end
   end
 end

lib/factory_girl/syntax/default.rb

@@ -43,8 +43,8 @@ module FactoryGirl
         end
 
         def factory(name, options = {}, &block)
-          factory = FactoryGirl.factory_by_name(name).allow_overrides
-          proxy = FactoryGirl::DefinitionProxy.new(factory.definition)
+          factory = FactoryGirl.factory_by_name(name)
+          proxy = FactoryGirl::DefinitionProxy.new(factory.definition.overridable)
           proxy.instance_eval(&block)
         end
       end

lib/factory_girl/trait.rb

@@ -12,12 +12,7 @@ module FactoryGirl
     end
 
     delegate :add_callback, :declare_attribute, :to_create, :define_trait,
-             :callbacks, :to => :@definition
-
-    def attributes
-      attribute_list.ensure_compiled
-      attribute_list
-    end
+             :callbacks, :attributes, :to => :@definition
 
     def names
       [@name]
@@ -30,11 +25,5 @@ module FactoryGirl
 
     protected
     attr_reader :block
-
-    private
-
-    def attribute_list
-      @definition.attribute_list
-    end
   end
 end

spec/factory_girl/attribute_list_spec.rb

@@ -1,14 +1,5 @@
 require "spec_helper"
 
-describe FactoryGirl::AttributeList, "overridable" do
-  it { should_not be_overridable }
-
-  it "can set itself as overridable" do
-    subject.overridable
-    subject.should be_overridable
-  end
-end
-
 describe FactoryGirl::AttributeList, "#define_attribute" do
   let(:static_attribute)  { FactoryGirl::Attribute::Static.new(:full_name, "value", false) }
   let(:dynamic_attribute) { FactoryGirl::Attribute::Dynamic.new(:email, false, lambda {|u| "#{u.full_name}@example.com" }) }
@@ -31,18 +22,6 @@ describe FactoryGirl::AttributeList, "#define_attribute" do
       2.times { subject.define_attribute(static_attribute) }
     }.to raise_error(FactoryGirl::AttributeDefinitionError, "Attribute already defined: full_name")
   end
-
-  context "when set as overridable" do
-    let(:static_attribute_with_same_name) { FactoryGirl::Attribute::Static.new(:full_name, "overridden value", false) }
-    before { subject.overridable }
-
-    it "redefines the attribute if the name already exists" do
-      subject.define_attribute(static_attribute)
-      subject.define_attribute(static_attribute_with_same_name)
-
-      subject.to_a.should == [static_attribute_with_same_name]
-    end
-  end
 end
 
 describe FactoryGirl::AttributeList, "#define_attribute with a named attribute list" do
@@ -60,7 +39,7 @@ describe FactoryGirl::AttributeList, "#define_attribute with a named attribute l
   end
 end
 
-describe FactoryGirl::AttributeList, "#apply_attribute_list" do
+describe FactoryGirl::AttributeList, "#apply_attributes" 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" }) }
@@ -74,20 +53,20 @@ describe FactoryGirl::AttributeList, "#apply_attribute_list" do
 
   it "prepends applied attributes" do
     subject.define_attribute(full_name_attribute)
-    subject.apply_attribute_list(list(city_attribute))
+    subject.apply_attributes(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_attribute_list(list(city_attribute, email_attribute))
+    subject.apply_attributes(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_attribute_list(list(city_attribute, email_attribute))
+    subject.apply_attributes(list(city_attribute, email_attribute))
     subject.to_a.should == [city_attribute, full_name_attribute, email_attribute, login_attribute]
   end
 
@@ -95,38 +74,7 @@ describe FactoryGirl::AttributeList, "#apply_attribute_list" do
     subject.define_attribute(full_name_attribute)
     attribute_with_same_name = FactoryGirl::Attribute::Static.new(:full_name, "Benjamin Franklin", false)
 
-    subject.apply_attribute_list(list(attribute_with_same_name))
+    subject.apply_attributes(list(attribute_with_same_name))
     subject.to_a.should == [full_name_attribute]
   end
-
-  context "when set as overridable" do
-    before { subject.overridable }
-
-    it "prepends applied attributes" do
-      subject.define_attribute(full_name_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_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_attribute_list(list(city_attribute, email_attribute))
-      subject.to_a.should == [city_attribute, full_name_attribute, email_attribute, login_attribute]
-    end
-
-    it "overwrites attributes that are already defined" do
-      subject.define_attribute(full_name_attribute)
-      attribute_with_same_name = FactoryGirl::Attribute::Static.new(:full_name, "Benjamin Franklin", false)
-
-      subject.apply_attribute_list(list(attribute_with_same_name))
-      subject.to_a.should == [attribute_with_same_name]
-    end
-  end
 end

spec/factory_girl/declaration_list_spec.rb

@@ -1,17 +1,71 @@
 require "spec_helper"
 
-describe FactoryGirl::DeclarationList, "#to_attributes" do
+describe FactoryGirl::DeclarationList, "#attribute_list" 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]
+  it "returns an AttributeList" do
+    subject.attribute_list.should be_a(FactoryGirl::AttributeList)
+  end
 
-    subject << dynamic_declaration
-    subject.to_attributes.should == [static_attribute_1, static_attribute_2, dynamic_attribute_1]
+  let(:attribute_list) { stub("attribute list", :define_attribute => true) }
+
+  it "defines each attribute on the attribute list" do
+    FactoryGirl::AttributeList.stubs(:new => attribute_list)
+
+    subject.declare_attribute(static_declaration)
+    subject.declare_attribute(dynamic_declaration)
+
+    subject.attribute_list
+
+    attribute_list.should have_received(:define_attribute).with(static_attribute_1)
+    attribute_list.should have_received(:define_attribute).with(static_attribute_2)
+    attribute_list.should have_received(:define_attribute).with(dynamic_attribute_1)
+  end
+
+  it "creates a new attribute list upon every invocation" do
+    subject.attribute_list.should_not == subject.attribute_list
+  end
+end
+
+describe FactoryGirl::DeclarationList, "#declare_attribute" do
+  let(:declaration_1)              { stub("declaration", :name => "declaration 1") }
+  let(:declaration_2)              { stub("declaration", :name => "declaration 2") }
+  let(:declaration_with_same_name) { stub("declaration", :name => "declaration 1") }
+
+  context "when not overridable" do
+    it "adds the declaration to the list" do
+      subject.declare_attribute(declaration_1)
+      subject.to_a.should == [declaration_1]
+
+      subject.declare_attribute(declaration_2)
+      subject.to_a.should == [declaration_1, declaration_2]
+    end
+  end
+
+  context "when overridable" do
+    before { subject.overridable }
+
+    it "adds the declaration to the list" do
+      subject.declare_attribute(declaration_1)
+      subject.to_a.should == [declaration_1]
+
+      subject.declare_attribute(declaration_2)
+      subject.to_a.should == [declaration_1, declaration_2]
+    end
+
+    it "deletes declarations with the same name" do
+      subject.declare_attribute(declaration_1)
+      subject.to_a.should == [declaration_1]
+
+      subject.declare_attribute(declaration_2)
+      subject.to_a.should == [declaration_1, declaration_2]
+
+      subject.declare_attribute(declaration_with_same_name)
+      subject.to_a.should == [declaration_2, declaration_with_same_name]
+    end
   end
 end

spec/factory_girl/definition_spec.rb

@@ -1,7 +1,8 @@
 require "spec_helper"
 
 describe FactoryGirl::Definition do
-  it { should delegate(:declare_attribute).to(:attribute_list) }
+  it { should delegate(:declare_attribute).to(:declarations) }
+  it { should delegate(:attributes).to(:declarations).as(:attribute_list) }
 end
 
 describe FactoryGirl::Definition, "with a name" do
@@ -9,9 +10,19 @@ describe FactoryGirl::Definition, "with a name" do
   subject    { FactoryGirl::Definition.new(name) }
 
   it "creates a new attribute list with the name passed" do
-    FactoryGirl::AttributeList.stubs(:new)
+    FactoryGirl::DeclarationList.stubs(:new)
     subject
-    FactoryGirl::AttributeList.should have_received(:new).with(name)
+    FactoryGirl::DeclarationList.should have_received(:new).with(name)
+  end
+end
+
+describe FactoryGirl::Definition, "#overridable" do
+  let(:list) { stub("declaration list", :overridable => true) }
+  before { FactoryGirl::DeclarationList.stubs(:new => list) }
+
+  it "sets the declaration list as overridable" do
+    subject.overridable.should == subject
+    list.should have_received(:overridable).once
   end
 end
 

spec/factory_girl/null_factory_spec.rb

@@ -3,6 +3,7 @@ require "spec_helper"
 describe FactoryGirl::NullFactory do
   it { should delegate(:defined_traits).to(:definition) }
   it { should delegate(:callbacks).to(:definition) }
+  it { should delegate(:attributes).to(:definition) }
 
   its(:compile)          { should be_nil }
   its(:default_strategy) { should be_nil }

spec/support/matchers/declaration.rb

@@ -21,7 +21,7 @@ module DeclarationMatchers
     end
 
     def matches?(subject)
-      subject.attribute_list.declarations.include?(expected_declaration)
+      subject.declarations.include?(expected_declaration)
     end
 
     def named(name)