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

Find factories registered for namespaced models #843

Closed
wants to merge 1 commit into from

Conversation

monkbroc
Copy link
Contributor

@monkbroc monkbroc commented Dec 7, 2015

Namespaced models like Invoice::Item should be able to be found by FactoryGirl.create(Invoice::Item) if registered as factory :invoice_item.

I added a test for this new behavior.

Fixes #740

Namespaced models like `Invoice::Item` should be able to be found by `FactoryGirl.create(Invoice::Item)` if registered as `factory :invoice_item`.

I added a test for this new behavior.

Fixes thoughtbot#740
@joshuaclayton
Copy link
Contributor

@monkbroc I'm not sure accessing factories by the class itself is something we want to promote. While I understand some amount of ease with using described_class in RSpec, I think it's a way of causing confusion, as there's not a 1:1 mapping between the classes.

factory :invoice_item, class: Invoice::Item do
  # base attributes

  factory :awesome_invoice_item do
    # custom attributes to make it awesome
  end
end

Both of these will build an Invoice::Item instance, but when running build(:invoice_item), which takes precedence, the last-declared or the one with the class? The fact that it's not obvious is concerning.

The other aspect is that there's a tight coupling when using classes (not that there wasn't before, but this change further enables class usage). Given:

factory :item, class: Invoice::Item

build(:item) works correctly, while build(Invoice::Item) does not. Because of the conditional nature of this, I do wonder if the more appropriate action would be to deprecate providing a class (to be removed in FG 5.0), and instead require a string or symbol directly matching how the factory key is registered.

I do appreciate you taking the time to solve this, but I do wonder if it's something that should rather be disallowed rather than further enabled. Thoughts?

@monkbroc
Copy link
Contributor Author

I agree that providing fewer variants of the FG API would be better. Deprecating finding a factory using a class is the way to go for FG 5.0. I can submit a PR for this if you'd like (don't know if FG 5 is on the horizon yet).

@joshuaclayton
Copy link
Contributor

@monkbroc if you'd like to tackle this, I'd love to see a PR. In the past, we've wrapped this sort of functionality in feature flags and included deprecations to help the upgrade process. Let me see if I can find some older features we'd done this for as examples.

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

Successfully merging this pull request may close these issues.

2 participants