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

Django ImageField is a SubFactory now, should it be? #451

Closed
olegpidsadnyi opened this issue Feb 2, 2018 · 4 comments
Closed

Django ImageField is a SubFactory now, should it be? #451

olegpidsadnyi opened this issue Feb 2, 2018 · 4 comments

Comments

@olegpidsadnyi
Copy link
Contributor

I've received an issue pytest-dev/pytest-factoryboy#58

pytest-factoryboy introspects the declarations in order to create fixtures automatically related to the current factory.

So far I'm comparing isinstance(decl, SubFactory) and isinstance(decl, RelatedFactory).
Now this breaks for the django.ImageField since it is a subclass of a Dict and also a SubFactory (really weird hierarchy). Is it not abused? Should it really be a SubFactory?
If ImageField is a SubFactory why doesn't it accept a factory as a parameter in order to respect the interface of the SubFactory (common OOP sense). If ImageField IS A SubFactory == False then why it won't use composition and invoke a dict factory inside the implementation?

I could check exact types like if type(decl, SubFactory) then create fixture for the model referenced by SubFactory. But this will prevent from subclassing a SubFactory in the valid cases.

Any suggestions?

@marky1991
Copy link

In the latest versions, Dict is a subclass of SubFactory. ImageFactory only inherits directly from Dict. Dict accepts a dict_factory if you really need that. Not sure if that helps.

@olegpidsadnyi
Copy link
Contributor Author

@marky1991 I introspect the attributes in order to generate fixtures. SubFactory means that there's a factory behind it. I think Dict is a factory that has a dict as a model. Dict isn't really a subfactory that has a factory of yet another factory that is just an indirection to come to the dict or an orderdereddict.

dict_factory as a parameter makes very little sense to me, model as a parameter would do more sense. Which dict you want to use. Honestly ordereddict doesn't make a lot of sense to me, because once you iterate over it's items it is no longer a dict. Dict is an interface with an index operator [].

Dict(ordered=True) or Dict(model_class=ordereddict) would make more sense than being a SubFactory.
What if I pass Dict(factory_class=List)? I think it is an architectural mistake to make Dict IS A SubFactory, which broke a user space.

The question is how far did it go and can it be undone?

@rbarrois
Copy link
Member

@olegpidsadnyi Indeed, the design wasn't very clean - mostly a shortcut to keep these fields working after the core refactory.

I've changed it to a cleaner and more explicit design, which should help with your issue.

On the other hand, I make no guarantee on the class names or hierarchy, beyond the factory.Factory class and its ORM-specific derivatives.

@olegpidsadnyi
Copy link
Contributor Author

I see.. Dict is a declaration and DictFactory is a factory...

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

No branches or pull requests

3 participants