Add attribute lists

lib/factory_girl.rb

@@ -14,6 +14,7 @@
 require 'factory_girl/attribute/implicit'
 require 'factory_girl/attribute/attribute_group'
 require 'factory_girl/sequence'
+require 'factory_girl/attribute_list'
 require 'factory_girl/attribute_group'
 require 'factory_girl/aliases'
 require 'factory_girl/definition_proxy'

lib/factory_girl/attribute_group.rb

@@ -1,40 +1,29 @@
 module FactoryGirl
-
   class AttributeGroup
     attr_reader :name
-    attr_reader :attributes
-
+
     def initialize(name, &block) #:nodoc:
       @name = name
-      @attributes = []
+      @attribute_list = AttributeList.new
+
       proxy = FactoryGirl::DefinitionProxy.new(self)
       proxy.instance_eval(&block) if block_given?
     end
 
     def define_attribute(attribute)
-      name = attribute.name
-      if attribute_defined?(name)
-        raise AttributeDefinitionError, "Attribute already defined: #{name}"
-      end
-      @attributes << attribute
+      @attribute_list.define_attribute(attribute)
     end
-    
+
     def add_callback(name, &block)
-      unless [:after_build, :after_create, :after_stub].include?(name.to_sym)
-        raise InvalidCallbackNameError, "#{name} is not a valid callback name. Valid callback names are :after_build, :after_create, and :after_stub"
-      end
-      @attributes << Attribute::Callback.new(name.to_sym, block)
+      @attribute_list.add_callback(name, &block)
     end
-
+
+    def attributes
+      @attribute_list.to_a
+    end
+
     def names
       [@name]
     end
-
-    private
-
-    def attribute_defined? (name)
-      !@attributes.detect {|attr| attr.name == name && !attr.is_a?(Attribute::Callback) }.nil?
-    end
-
   end
 end

lib/factory_girl/attribute_list.rb

@@ -0,0 +1,59 @@
+module FactoryGirl
+  class AttributeList
+    include Enumerable
+
+    def initialize
+      @attributes = []
+    end
+
+    def define_attribute(attribute)
+      if attribute_defined?(attribute.name)
+        raise AttributeDefinitionError, "Attribute already defined: #{attribute.name}"
+      end
+
+      @attributes << attribute
+    end
+
+    def add_callback(name, &block)
+      unless valid_callback_names.include?(name.to_sym)
+        raise InvalidCallbackNameError, "#{name} is not a valid callback name. Valid callback names are #{valid_callback_names.inspect}"
+      end
+
+      @attributes << Attribute::Callback.new(name.to_sym, block)
+    end
+
+    def each(&block)
+      @attributes.each(&block)
+    end
+
+    def attribute_defined?(attribute_name)
+      !@attributes.detect do |attribute|
+        attribute.name == attribute_name &&
+          !attribute.is_a?(FactoryGirl::Attribute::Callback)
+      end.nil?
+    end
+
+    def apply_attributes(attributes_to_apply)
+      new_attributes = []
+
+      attributes_to_apply.each do |attribute|
+        if attribute_defined?(attribute.name)
+          @attributes.delete_if do |attrib|
+            new_attributes << attrib.clone if attrib.name == attribute.name
+          end
+        else
+          new_attributes << attribute.clone
+        end
+      end
+
+      @attributes.unshift *new_attributes
+      @attributes = @attributes.partition {|attr| attr.priority.zero? }.flatten
+    end
+
+    private
+
+    def valid_callback_names
+      [:after_build, :after_create, :after_stub]
+    end
+  end
+end

lib/factory_girl/factory.rb

@@ -13,7 +13,6 @@ class DuplicateDefinitionError < RuntimeError
 
   class Factory
     attr_reader :name #:nodoc:
-    attr_reader :attributes #:nodoc:
     attr_reader :attribute_groups #:nodoc:
 
     def factory_name
@@ -38,7 +37,7 @@ def initialize(name, options = {}) #:nodoc:
       @name             = factory_name_for(name)
       @parent           = options[:parent]
       @options          = options
-      @attributes       = []
+      @attribute_list   = AttributeList.new
       @attribute_groups = []
     end
 
@@ -47,43 +46,43 @@ def inherit_from(parent) #:nodoc:
       @options[:default_strategy] ||= parent.default_strategy
 
       apply_attributes(parent.attributes)
-      sort_attributes!
     end
 
-    def apply_attribute_groups(groups)
+    def apply_attribute_groups(groups) #:nodoc:
       groups.reverse.map { |name| attribute_group_by_name(name) }.each do |group|
         apply_attributes(group.attributes)
       end
-      sort_attributes!
+    end
+
+    def apply_attributes(attributes_to_apply)
+      @attribute_list.apply_attributes(attributes_to_apply)
     end
 
     def define_attribute(attribute)
-      name = attribute.name
-      # TODO: move these checks into Attribute
-      if attribute_defined?(name)
-        raise AttributeDefinitionError, "Attribute already defined: #{name}"
-      end
       if attribute.respond_to?(:factory) && attribute.factory == self.name
-        raise AssociationDefinitionError, "Self-referencing association '#{name}' in factory '#{self.name}'"
+        raise AssociationDefinitionError, "Self-referencing association '#{attribute.name}' in factory '#{self.name}'"
       end
-      @attributes << attribute
+
+      @attribute_list.define_attribute(attribute)
     end
 
     def define_attribute_group(group)
       @attribute_groups << group
     end
 
     def add_callback(name, &block)
-      unless [:after_build, :after_create, :after_stub].include?(name.to_sym)
-        raise InvalidCallbackNameError, "#{name} is not a valid callback name. Valid callback names are :after_build, :after_create, and :after_stub"
-      end
-      @attributes << Attribute::Callback.new(name.to_sym, block)
+      @attribute_list.add_callback(name, &block)
+    end
+
+
+    def attributes
+      @attribute_list.to_a
     end
 
     def run(proxy_class, overrides) #:nodoc:
       proxy = proxy_class.new(build_class)
       overrides = symbolize_keys(overrides)
-      @attributes.each do |attribute|
+      @attribute_list.each do |attribute|
         factory_overrides = overrides.select { |attr, val| attribute.aliases_for?(attr) }
         if factory_overrides.empty?
           attribute.add_to(proxy)
@@ -167,10 +166,6 @@ def factory_name_for(class_or_to_s)
       end
     end
 
-    def attribute_defined? (name)
-      !@attributes.detect {|attr| attr.name == name && !attr.is_a?(Attribute::Callback) }.nil?
-    end
-
     def assert_valid_options(options)
       invalid_keys = options.keys - [:class, :parent, :default_strategy, :aliases, :attribute_groups]
       unless invalid_keys == []
@@ -213,26 +208,6 @@ def symbolize_keys(hash)
       end
     end
 
-    def apply_attributes(attributes_to_apply)
-      new_attributes=[]
-
-      attributes_to_apply.each do |attribute|
-        if attribute_defined?(attribute.name)
-          @attributes.delete_if do |attrib|
-            new_attributes << attrib.clone if attrib.name == attribute.name
-          end
-        else
-          new_attributes << attribute.clone
-        end
-      end
-
-      @attributes.unshift *new_attributes
-    end
-
-    def sort_attributes!
-      @attributes = @attributes.partition {|attr| attr.priority.zero? }.flatten
-    end
-
     def attribute_group_for(name)
       attribute_groups.detect {|attribute_group| attribute_group.name == name }
     end

spec/factory_girl/attribute_list_spec.rb

@@ -0,0 +1,111 @@
+require "spec_helper"
+
+describe FactoryGirl::AttributeList, "#define_attribute" do
+  let(:static_attribute)  { FactoryGirl::Attribute::Static.new(:full_name, "value") }
+  let(:dynamic_attribute) { FactoryGirl::Attribute::Dynamic.new(:email, lambda {|u| "#{u.full_name}@example.com" }) }
+
+  it "maintains a list of attributes" do
+    subject.define_attribute(static_attribute)
+    subject.to_a.should == [static_attribute]
+
+    subject.define_attribute(dynamic_attribute)
+    subject.to_a.should == [static_attribute, dynamic_attribute]
+  end
+
+  it "raises if an attribute has already been defined" do
+    expect {
+      2.times { subject.define_attribute(static_attribute) }
+    }.to raise_error(FactoryGirl::AttributeDefinitionError, "Attribute already defined: full_name")
+  end
+end
+
+describe FactoryGirl::AttributeList, "#attribute_defined?" do
+  let(:static_attribute)                    { FactoryGirl::Attribute::Static.new(:full_name, "value") }
+  let(:callback_attribute)                  { FactoryGirl::Attribute::Callback.new(:after_create, lambda { }) }
+  let(:static_attribute_named_after_create) { FactoryGirl::Attribute::Static.new(:after_create, "funky!") }
+
+  it "knows if an attribute has been defined" do
+    subject.attribute_defined?(static_attribute.name).should == false
+
+    subject.define_attribute(static_attribute)
+
+    subject.attribute_defined?(static_attribute.name).should == true
+    subject.attribute_defined?(:undefined_attribute).should == false
+  end
+
+  it "doesn't reference callbacks" do
+    subject.define_attribute(callback_attribute)
+
+    subject.attribute_defined?(:after_create).should == false
+
+    subject.define_attribute(static_attribute_named_after_create)
+    subject.attribute_defined?(:after_create).should == true
+  end
+end
+
+describe FactoryGirl::AttributeList, "#add_callback" do
+  let(:proxy_class) { mock("klass") }
+  let(:proxy) { FactoryGirl::Proxy.new(proxy_class) }
+  let(:valid_callback_names) { [:after_create, :after_build, :after_stub] }
+  let(:invalid_callback_names) { [:before_create, :before_build, :bogus] }
+
+
+  it "allows for defining adding a callback" do
+    subject.add_callback(:after_create) { "Called after_create" }
+
+    subject.first.name.should == :after_create
+
+    subject.first.add_to(proxy)
+    proxy.callbacks[:after_create].first.call.should == "Called after_create"
+  end
+
+  it "allows valid callback names to be assigned" do
+    valid_callback_names.each do |callback_name|
+      expect do
+        subject.add_callback(callback_name) { "great name!" }
+      end.to_not raise_error(FactoryGirl::InvalidCallbackNameError)
+    end
+  end
+
+  it "raises if an invalid callback name is assigned" do
+    invalid_callback_names.each do |callback_name|
+      expect do
+        subject.add_callback(callback_name) { "great name!" }
+      end.to raise_error(FactoryGirl::InvalidCallbackNameError, "#{callback_name} is not a valid callback name. Valid callback names are [:after_build, :after_create, :after_stub]")
+    end
+  end
+end
+
+describe FactoryGirl::AttributeList, "#apply_attributes" do
+  let(:full_name_attribute) { FactoryGirl::Attribute::Static.new(:full_name, "John Adams") }
+  let(:city_attribute)      { FactoryGirl::Attribute::Static.new(:city, "Boston") }
+  let(:email_attribute)     { FactoryGirl::Attribute::Dynamic.new(:email, lambda {|model| "#{model.full_name}@example.com" }) }
+  let(:login_attribute)     { FactoryGirl::Attribute::Dynamic.new(:login, lambda {|model| "username-#{model.full_name}" }) }
+
+  it "prepends applied attributes" do
+    subject.define_attribute(full_name_attribute)
+    subject.apply_attributes([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([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([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")
+
+    subject.apply_attributes([attribute_with_same_name])
+    subject.to_a.should == [attribute_with_same_name]
+  end
+end

spec/factory_girl/factory_spec.rb

@@ -25,24 +25,6 @@
     @factory.default_strategy.should == :create
   end
 
-  it "should not allow the same attribute to be added twice" do
-    lambda {
-      2.times { @factory.define_attribute FactoryGirl::Attribute::Static.new(:name, 'value') }
-    }.should raise_error(FactoryGirl::AttributeDefinitionError)
-  end
-
-  it "should add a callback attribute when defining a callback" do
-    mock(FactoryGirl::Attribute::Callback).new(:after_create, is_a(Proc)) { 'after_create callback' }
-    @factory.add_callback(:after_create) {}
-    @factory.attributes.should include('after_create callback')
-  end
-
-  it "should raise an InvalidCallbackNameError when defining a callback with an invalid name" do
-    lambda{
-      @factory.add_callback(:invalid_callback_name) {}
-    }.should raise_error(FactoryGirl::InvalidCallbackNameError)
-  end
-
   describe "after adding an attribute" do
     before do
       @attribute = "attribute"