- Notifications
You must be signed in to change notification settings - Fork302
Add support for GenericRelations#319
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Add support for GenericRelations#319
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Could you call thisTaggedItemFactory instead ofEntryTaggedItemFactory? 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
After reviewing this package's code, imports are usually alphabetically sorted. Could you move thesecontrib imports about thedb 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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?
ifdjango.VERSION>= (1,9):# ... snip unmodified lines ...fromdjango.contrib.contenttypes.fieldsimportReverseGenericManyToOneDescriptorelse:fromdjango.contrib.contenttypes.fieldsimportReverseGenericRelatedObjectsDescriptorasReverseGenericManyToOneDescriptor
I 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 theelse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done!
| @@ -2,6 +2,6 @@ | |||
| pytest>=2.9.0,<3.0 | |||
| pytest-django | |||
| pytest-factoryboy | |||
| fake-factory | |||
| Faker | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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-io commentedFeb 23, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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! |
PRdjango-json-api#319 brought support for generic relations. Unfortunately apps thatdon'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 saferalternative to `type(something) is type(None)`. If `something` happenedto be `None` we would enter a branch that was never supposed to run.
PRdjango-json-api#319 brought support for generic relations. Unfortunately apps thatdon'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 saferalternative to `type(something) is type(None)`. If `something` happenedto be `None` we would enter a branch that was never supposed to run.
PR#319 brought support for generic relations. Unfortunately apps thatdon'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 saferalternative to `type(something) is type(None)`. If `something` happenedto be `None` we would enter a branch that was never supposed to run.
Uh oh!
There was an error while loading.Please reload this page.
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 toFaker