Add support for GenericRelations #319
Conversation
santiavenda2
changed the title
example/factories/__init__.py
Outdated
| @@ -58,3 +58,11 @@ class Meta: | |||
| body = factory.LazyAttribute(lambda x: faker.text()) | |||
| author = factory.SubFactory(AuthorFactory) | |||
|
|
|||
|
|
|||
| class EntryTaggedItemFactory(factory.django.DjangoModelFactory): | |||
There was a problem hiding this comment.
Could you call this TaggedItemFactory instead of EntryTaggedItemFactory? That seems more in line with the other factory naming conventions here.
example/models.py
Outdated
| @@ -3,6 +3,9 @@ | |||
|
|
|||
| from django.db import models | |||
| from django.utils.encoding import python_2_unicode_compatible | |||
| from django.contrib.contenttypes.models import ContentType | |||
There was a problem hiding this comment.
After reviewing this package's code, imports are usually alphabetically sorted. Could you move these contrib imports about the db import?
| @@ -30,11 +30,14 @@ | |||
| if django.VERSION >= (1, 9): | |||
| from django.db.models.fields.related_descriptors import ManyToManyDescriptor, ReverseManyToOneDescriptor | |||
| ReverseManyRelatedObjectsDescriptor = type(None) | |||
| from django.contrib.contenttypes.fields import ReverseGenericManyToOneDescriptor | |||
There was a problem hiding this comment.
This method of having two mirrored imports that set one to the None type is something I found confusing. After digging through the Django contenttypes source, I see that what is happening is trying to deal with a rename of the descriptor from 1.8 to 1.9. Unfortunately, this method leads to extra branches in an already large if/elif section.
How about using an import alias instead?
if django.VERSION >= (1, 9):
# ... snip unmodified lines ...
from django.contrib.contenttypes.fields import ReverseGenericManyToOneDescriptor
else:
from django.contrib.contenttypes.fields import ReverseGenericRelatedObjectsDescriptor as ReverseGenericManyToOneDescriptorI believe this would clean up the code, be more efficient, and capture the fact that the descriptor was renamed. It seems this pattern was already followed for the first two imports that follow the else.
| @@ -2,6 +2,6 @@ | |||
| pytest>=2.9.0,<3.0 | |||
| pytest-django | |||
| pytest-factoryboy | |||
| fake-factory | |||
| Faker | |||
There was a problem hiding this comment.
Thanks for picking up this package name change. This change has bit me on a few projects already.
|
Thanks @santiavenda2 for the contribution! Making this work for This feels like a feature worth documenting. Would you be able to do some of that? At the very least, I think starting a v2.X entry with a bullet point about I'd be happy to merge after some minor modifications occur. 👍 |
Codecov Report
@@ Coverage Diff @@
## develop #319 +/- ##
===========================================
+ Coverage 91.58% 91.64% +0.06%
===========================================
Files 49 50 +1
Lines 2318 2359 +41
===========================================
+ Hits 2123 2162 +39
- Misses 195 197 +2
Continue to review full report at Codecov.
|
|
Nice work. Thanks, @santiavenda2! |
PR django-json-api#319 brought support for generic relations. Unfortunately apps that don't add contenttypes to it's INSTALLED_APPS would crash and burn: ``` RuntimeError: Model class django.contrib.contenttypes.models.ContentType doesn't declare an explicit app_label and isn't in an application in INSTALLED_APPS. ``` Also using `type(something) is object()` comparison as a safer alternative to `type(something) is type(None)`. If `something` happened to be `None` we would enter a branch that was never supposed to run.
PR django-json-api#319 brought support for generic relations. Unfortunately apps that don't add contenttypes to it's INSTALLED_APPS would crash and burn: ``` RuntimeError: Model class django.contrib.contenttypes.models.ContentType doesn't declare an explicit app_label and isn't in an application in INSTALLED_APPS. ``` Also using `type(something) is object()` comparison as a safer alternative to `type(something) is type(None)`. If `something` happened to be `None` we would enter a branch that was never supposed to run.
PR #319 brought support for generic relations. Unfortunately apps that don't add contenttypes to it's INSTALLED_APPS would crash and burn: ``` RuntimeError: Model class django.contrib.contenttypes.models.ContentType doesn't declare an explicit app_label and isn't in an application in INSTALLED_APPS. ``` Also using `type(something) is object()` comparison as a safer alternative to `type(something) is type(None)`. If `something` happened to be `None` we would enter a branch that was never supposed to run.
Fixed #224
Also, we fixed
ConftestImportFailure: (local('/home/santiago/develop/invgate-django-rest-framework-json-api/example/tests/conftest.py'), (<type 'exceptions.ImportError'>, ImportError('No module named faker',), <traceback object at 0x7f64f3d8c5a8>))Fake-factory has been deprecated, changing its name to Faker