WIP: initial cleanup

lib/factory_girl.rb

@@ -64,5 +64,4 @@ module FactoryGirl
   def self.attribute_group_by_name(name)
     attribute_groups.find(name)
   end
-
 end

lib/factory_girl/attribute/attribute_group.rb

@@ -4,17 +4,18 @@ module FactoryGirl
     class AttributeGroup < Attribute
       def initialize(name, factory)
         super(name)
-        @factory=factory
+        @factory = factory
       end
 
       def add_to(proxy)
-        if @factory
-          @factory.attribute_group_by_name(name).attributes.each { |attr| attr.add_to(proxy) }
-        else
-          FactoryGirl.attribute_group_by_name(name).attributes.each { |attr| attr.add_to(proxy) }
-        end
-      end
+        attribute_group.attributes.each { |attr| attr.add_to(proxy) }
       end
 
+      private
+
+      def attribute_group
+        (@factory || FactoryGirl).attribute_group_by_name(name)
+      end
+    end
   end
 end

lib/factory_girl/attribute/implicit.rb

@@ -2,9 +2,9 @@ module FactoryGirl
   class Attribute
 
     class Implicit < Attribute
-      def initialize(name, factory=nil)
+      def initialize(name, factory = nil)
         super(name)
-        @factory=factory
+        @factory = factory
       end
 
       def add_to(proxy)

lib/factory_girl/factory.rb

@@ -44,36 +44,15 @@ module FactoryGirl
       @options[:class]            ||= parent.class_name
       @options[:default_strategy] ||= parent.default_strategy
 
-      new_attributes = []
-      parent.attributes.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
+      apply_attributes(parent.attributes)
+      sort_attributes!
     end
 
     def apply_attribute_groups(groups)
-      groups.reverse.map{ |name| attribute_group_by_name(name) }.each do |group|
-        new_attributes=[]
-        group.attributes.each do |attribute|
-          if attribute_defined?(attribute.name)
-            @attributes.delete_if do |attrib|
-              new_attributes << attrib.clone if attrib.name == attribute.name
+      groups.reverse.map { |name| attribute_group_by_name(name) }.each do |group|
+        apply_attributes(group.attributes)
       end
-          else
-            new_attributes << attribute.clone
-          end
-        end
-        @attributes.unshift *new_attributes
-      end
-      @attributes = @attributes.partition{|attr| attr.priority.zero?}.flatten
+      sort_attributes!
     end
 
     def define_attribute(attribute)
@@ -125,10 +104,10 @@ module FactoryGirl
     def attribute_group_by_name(name)
       return attribute_groups.find(name) if attribute_groups.registered?(name)
 
-      if @parent.nil?
-        FactoryGirl::attribute_group_by_name(name)
-      else
+      if @parent
         FactoryGirl.factory_by_name(@parent).attribute_group_by_name(name)
+      else
+        FactoryGirl.attribute_group_by_name(name)
       end
     end
 
@@ -232,6 +211,26 @@ module FactoryGirl
       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_groups
       @attribute_groups ||= Registry.new
     end

spec/acceptance/attribute_groups_spec.rb

@@ -3,9 +3,16 @@ require "acceptance/acceptance_helper"
 
 describe "an instance generated by a factory with multiple attribute groups" do
   before do
-    define_model("User", :name => :string, :admin => :boolean, :gender => :string, :email => :string)
+    define_model("User",
+                 :name   => :string,
+                 :admin  => :boolean,
+                 :gender => :string,
+                 :email  => :string)
 
     FactoryGirl.define do
+      factory :user_without_admin_scoping, :class => User do
+        admin_attribute_group
+      end
 
       factory :user do
         name "John"
@@ -14,6 +21,10 @@ describe "an instance generated by a factory with multiple attribute groups" do
           admin true
         end
 
+        attribute_group :admin_attribute_group do
+          admin true
+        end
+
         attribute_group :male do
           name   "Joe"
           gender "Male"
@@ -24,17 +35,21 @@ describe "an instance generated by a factory with multiple attribute groups" do
           gender "Female"
         end
 
-        factory :admin, :attribute_groups=>[:admin]
+        factory :admin, :attribute_groups => [:admin]
+
         factory :male_user do
           male
         end
+
         factory :female, :attribute_groups => [:female] do
           attribute_group :admin do
             admin true
             name "Judy"
           end
-          factory :female_admin_judy, :attribute_groups=>[:admin]
+
+          factory :female_admin_judy, :attribute_groups => [:admin]
         end
+
         factory :female_admin,            :attribute_groups => [:female, :admin]
         factory :female_after_male_admin, :attribute_groups => [:male, :female, :admin]
         factory :male_after_female_admin, :attribute_groups => [:female, :male, :admin]
@@ -44,10 +59,9 @@ describe "an instance generated by a factory with multiple attribute groups" do
         email { "#{name}@example.com" }
       end
 
-      factory :user_with_email, :class=>User, :attribute_groups=>[:email] do
+      factory :user_with_email, :class => User, :attribute_groups => [:email] do
         name "Bill"
       end
-      
     end
   end
 
@@ -89,7 +103,6 @@ describe "an instance generated by a factory with multiple attribute groups" do
   context "the child with multiple attribute groups who override the same attribute" do
     context "when the male assigns name after female" do
       subject      { FactoryGirl.create(:male_after_female_admin) }
-  
       its(:name)   { should == "Joe" }
       its(:gender) { should == "Male" }
       it           { should be_admin }
@@ -97,7 +110,6 @@ describe "an instance generated by a factory with multiple attribute groups" do
 
     context "when the female assigns name after male" do
       subject      { FactoryGirl.create(:female_after_male_admin) }
-      
       its(:name)   { should == "Jane" }
       its(:gender) { should == "Female" }
       it           { should be_admin }
@@ -122,4 +134,8 @@ describe "an instance generated by a factory with multiple attribute groups" do
     its(:gender) { should == "Male" }
   end
 
+  context "factory outside of scope" do
+    subject     { FactoryGirl.create(:user_without_admin_scoping) }
+    it { expect { subject }.to raise_error(ArgumentError, "Not registered: admin_attribute_group") }
+  end
 end

spec/factory_girl/definition_proxy_spec.rb

@@ -130,7 +130,7 @@ describe FactoryGirl::DefinitionProxy do
     name = :user
     attr = 'attribute'
     stub(attr).name { name }
-    mock(FactoryGirl::Attribute::Implicit).new(name,factory) { attr }
+    mock(FactoryGirl::Attribute::Implicit).new(name, factory) { attr }
     subject.send(name)
     factory.attributes.should include(attr)
   end