Allow DjangoModelFactories to use 'app.Model'. #66

Closed
mjtamlyn opened this Issue Jul 3, 2013 · 4 comments

Comments

Projects
None yet
2 participants
@mjtamlyn

mjtamlyn commented Jul 3, 2013

As moved over from dnerdy#9

@rbarrois said

By the way, I'm strongly against the proposed behavior:

  • Django's way could change in the future, and requires a single model for each app
  • factory_boy aims to handle more situations, ORMs and frameworks than just Django, and should keep a consistent behavior across all projects.

The convention of identifying model instances by app.ModelName is certainly not going anywhere in Django at present - I'm quite familiar with the app-refactor branch and this does not attempt to change this convention at all. As an example, consider the recently introduced AUTH_USER_MODEL setting which follows this convention.

I understand that factory_boy aims to support multiple ORMs etc, which is why the patch on the other project is applied only to DjangoModelFactory, which is already Django specific.

My main reason for doing this is that my factories.py have a large number of factories defined in them, and I generally do not require the model class for anything other than setting FACTORY_FOR - it results in a lot of imports to the file which aren't really needed.

If you are willing to reconsider your opposition to the proposed behaviour, then I can upgrade the patch apporpriately.

@rbarrois

This comment has been minimized.

Show comment
Hide comment
@rbarrois

rbarrois Jul 3, 2013

Member

Hi,

Thanks for the clarification. I had misread your initial proposal, and thought you intended to use the 'app.Model' pattern for the "late import" feature of factory.SubFactory() ; I hadn't considered using it in FACTORY_FOR.

Using two options for settings FACTORY_FOR goes against the zen of Python, but along the expected Django way.

I tend to create a factories.py file for each app, so my imports are kept simple:

import factory
from . import models

class MyModelFactory(factory.django.DjangoModelFactory):
    FACTORY_FOR = models.MyModel

Could you give an example of a code layout where using 'app.Model' helps?

Member

rbarrois commented Jul 3, 2013

Hi,

Thanks for the clarification. I had misread your initial proposal, and thought you intended to use the 'app.Model' pattern for the "late import" feature of factory.SubFactory() ; I hadn't considered using it in FACTORY_FOR.

Using two options for settings FACTORY_FOR goes against the zen of Python, but along the expected Django way.

I tend to create a factories.py file for each app, so my imports are kept simple:

import factory
from . import models

class MyModelFactory(factory.django.DjangoModelFactory):
    FACTORY_FOR = models.MyModel

Could you give an example of a code layout where using 'app.Model' helps?

@mjtamlyn

This comment has been minimized.

Show comment
Hide comment
@mjtamlyn

mjtamlyn Jul 4, 2013

I tend to keep my tests in a separate tests directory, with normally a single factories.py. This makes it a bit easier to keep all of the dependencies between my apps and their factories together. As each factory is often quite simple, I don't find the several hundred line file that unweildly. Because I've got models from multiple apps in the same file, the from . import models approach is less effective as I need to use as statements for each app, or import all the models individually. I prefer being able to use from . import factories in my test cases (which are not stored under each app).

To me this is a very "Djangoy" approach. As far as I can tell, there are almost no cases in Django when you cannot use this way of referring to a model, and I would view it as an expected API.

mjtamlyn commented Jul 4, 2013

I tend to keep my tests in a separate tests directory, with normally a single factories.py. This makes it a bit easier to keep all of the dependencies between my apps and their factories together. As each factory is often quite simple, I don't find the several hundred line file that unweildly. Because I've got models from multiple apps in the same file, the from . import models approach is less effective as I need to use as statements for each app, or import all the models individually. I prefer being able to use from . import factories in my test cases (which are not stored under each app).

To me this is a very "Djangoy" approach. As far as I can tell, there are almost no cases in Django when you cannot use this way of referring to a model, and I would view it as an expected API.

@rbarrois

This comment has been minimized.

Show comment
Hide comment
@rbarrois

rbarrois Jul 10, 2013

Member

That's indeed a reasonable use case.

Such a patch should allow for other ORMs / frameworks to define custom "associated class" loaders, though.

Member

rbarrois commented Jul 10, 2013

That's indeed a reasonable use case.

Such a patch should allow for other ORMs / frameworks to define custom "associated class" loaders, though.

@mjtamlyn

This comment has been minimized.

Show comment
Hide comment
@mjtamlyn

mjtamlyn Jul 11, 2013

The original patch overrides _discover_associated_class to look the model up if it's a string. I don't know what equivalents to django.db.models.loading.get_model exist for other ORM systems so it would be hard for me to write an appropriate patch.

We could in theory use full dotted paths to classes which would be more generic, but be a little more verbose for Django users.

The original patch overrides _discover_associated_class to look the model up if it's a string. I don't know what equivalents to django.db.models.loading.get_model exist for other ORM systems so it would be hard for me to write an appropriate patch.

We could in theory use full dotted paths to classes which would be more generic, but be a little more verbose for Django users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment