Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
implement requested changes
  • Loading branch information
santiavenda2 committed Feb 23, 2017
commit f9bfcc96f99e0d4b2bf6ad7b338f33d716c6fb37
2 changes: 1 addition & 1 deletion example/factories/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ class Meta:
author = factory.SubFactory(AuthorFactory)


class EntryTaggedItemFactory(factory.django.DjangoModelFactory):
class TaggedItemFactory(factory.django.DjangoModelFactory):

class Meta:
model = TaggedItem
Expand Down
4 changes: 2 additions & 2 deletions example/models.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
# -*- encoding: utf-8 -*-
from __future__ import unicode_literals

from django.db import models
from django.utils.encoding import python_2_unicode_compatible
from django.contrib.contenttypes.models import ContentType
Copy link
Collaborator

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 these contrib imports about the db import?

from django.contrib.contenttypes.fields import GenericForeignKey
from django.contrib.contenttypes.fields import GenericRelation
from django.db import models
from django.utils.encoding import python_2_unicode_compatible


class BaseModel(models.Model):
Expand Down
8 changes: 4 additions & 4 deletions example/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,22 @@
from pytest_factoryboy import register

from example.factories import BlogFactory, AuthorFactory, AuthorBioFactory, EntryFactory, CommentFactory, \
EntryTaggedItemFactory
TaggedItemFactory

register(BlogFactory)
register(AuthorFactory)
register(AuthorBioFactory)
register(EntryFactory)
register(CommentFactory)
register(EntryTaggedItemFactory)
register(TaggedItemFactory)


@pytest.fixture
def single_entry(blog, author, entry_factory, comment_factory, entry_tagged_item_factory):
def single_entry(blog, author, entry_factory, comment_factory, tagged_item_factory):

entry = entry_factory(blog=blog, authors=(author,))
comment_factory(entry=entry)
entry_tagged_item_factory(content_object=entry)
tagged_item_factory(content_object=entry)
return entry


Expand Down
12 changes: 6 additions & 6 deletions rest_framework_json_api/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,12 @@
from django.db.models.fields.related_descriptors import ManyToManyDescriptor, ReverseManyToOneDescriptor
ReverseManyRelatedObjectsDescriptor = type(None)
from django.contrib.contenttypes.fields import ReverseGenericManyToOneDescriptor
Copy link
Collaborator

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?

if django.VERSION >= (1, 9):
    # ... snip unmodified lines ...
    from django.contrib.contenttypes.fields import ReverseGenericManyToOneDescriptor
else:
    from django.contrib.contenttypes.fields import ReverseGenericRelatedObjectsDescriptor as ReverseGenericManyToOneDescriptor

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 the else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

ReverseGenericRelatedObjectsDescriptor = type(None)
else:
from django.db.models.fields.related import ManyRelatedObjectsDescriptor as ManyToManyDescriptor
from django.db.models.fields.related import ForeignRelatedObjectsDescriptor as ReverseManyToOneDescriptor
from django.db.models.fields.related import ReverseManyRelatedObjectsDescriptor
from django.contrib.contenttypes.fields import ReverseGenericRelatedObjectsDescriptor
ReverseGenericManyToOneDescriptor = type(None)
from django.contrib.contenttypes.fields import ReverseGenericRelatedObjectsDescriptor as ReverseGenericManyToOneDescriptor


def get_resource_name(context):
"""
Expand Down Expand Up @@ -226,9 +225,10 @@ def get_related_resource_type(relation):
elif parent_model_relation_type is ReverseManyRelatedObjectsDescriptor:
relation_model = parent_model_relation.field.related.model
elif parent_model_relation_type is ReverseGenericManyToOneDescriptor:
relation_model = parent_model_relation.rel.model
elif parent_model_relation_type is ReverseGenericRelatedObjectsDescriptor:
relation_model = parent_model_relation.field.related_model
if django.VERSION >= (1, 9):
relation_model = parent_model_relation.rel.model
else:
relation_model = parent_model_relation.field.related_model
else:
return get_related_resource_type(parent_model_relation)

Expand Down