Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add multiple inheritance support with parents #132

Closed
leandrocosta opened this issue Mar 23, 2011 · 9 comments
Closed

add multiple inheritance support with parents #132

leandrocosta opened this issue Mar 23, 2011 · 9 comments
Labels

Comments

@leandrocosta
Copy link

example:

Factory.define :user_expired_admin, :parents => [:admin, :user_expired] do |f|
end
@joshuaclayton
Copy link
Contributor

Hey,

This sounds like a pretty awesome feature; get me a pull request with some tests and I'll get it merged in! You're going to want to add it to lib/factory_girl/syntax/default.rb (not lib/factory_girl/syntax/vintage.rb)

Thanks!

@joshuaclayton
Copy link
Contributor

I started work in this branch (https://github.com/thoughtbot/factory_girl/tree/allow-for-multiple-parents), but I have two failures. It looks like if child factories are defined, their implied attributes are being copied immediately. In my tests, that means that once admin is defined as a factory, parent attributes (name in particular) are copied into it instead of lazily evaluated. This means that after the admin factory is evaluated, the parent's name attribute (John) is copied into it and is eventually applied to the factory that has three parents defined. This isn't expected and should be fixed before merging.

@twalpole
Copy link
Contributor

twalpole commented Aug 9, 2011

Instead of allowing for multiple factories to be used as parents, which brings along the issue of the parents parent being applied in the incorrect order since each parent loads it, how about something like "mixins" which would just be sets of attributes that do not inherit a parent and could be defined globally or within a factory for scoping purposes - this would change the example in the parents_spec into something like below

FactoryGirl.define do
  factory :user do
    name "John"

    mixin :admin do
      admin true
    end

    mixin :male do
      gender "Male"
    end

    mixin :female do
      name "Jane"
      gender "Female"
    end

    factory :admin, :mixins=>[:admin]
    factory :male, :mixins=>[:male]
    factory :female, :mixins=>[:female]
    factory :female_admin, :mixins => [:female, :admin]
    factory :female_after_male_admin, :mixins => [:male, :female, :admin]
    factory :male_after_female_admin, :mixins => [:female, :male, :admin]
  end

@joshuaclayton
Copy link
Contributor

@twalpole I like this a bit more. I think I'd lean more towards attribute_group than mixin since it may cause some confusion with Module#mix. However, I do like this route of defining a group of attributes and applying them sequentially to a factory. I'm for scoping these groups too, much like sequences, and allowing for a global scope as well (for polymorphic models etc.) Did you want to tackle an initial implementation? I don't mind scrapping my current work and going this route if you don't want to / have time

@twalpole
Copy link
Contributor

twalpole commented Aug 9, 2011

Sure, I can try an initial implementation this evening

@twalpole
Copy link
Contributor

@joshuaclayton I have pushed an initial implementation into the attribute_groups branch in my fork https://github.com/twalpole/factory_girl/tree/attribute_groups . The specs are based off the specs from your WIP and are all passing correctly currently. I need to add more specs and implementation for global attribute groups, but feedback on the current code would be appreciated. One more thing I thought that might be nice would be add a :factory option to the attr_group instruction that would allow for creating a factory directly from the attr_group. In the code below this would then create 3 factories (user, admin, male_admin) - thoughts?

factory :user do
  name "John"
  attr_group :admin, :factory=>true do
    admin true
  end
  attr_group :male do
    gender "Male"
  end
  factory :male_admin, :attr_groups=>[:male, :admin]
end

@twalpole
Copy link
Contributor

@joshuaclayton I submitted a pull request implementing this functionality

@joshuaclayton
Copy link
Contributor

Cool - I'll comment on the pull request, thanks!

@joshuaclayton
Copy link
Contributor

This was merged into master; the work is here: 40d08ee...9dcb209

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants