Search code, repositories, users, issues, pull requests...

Continue to review full report at Codecov.


 Fix included resources not being included on list

77868fc

@codecov-io

Copy link

codecov-io commented Dec 5, 2016 •

edited

Loading

Codecov Report

Merging #307 into develop will increase coverage by 0.02%.
The diff coverage is 100%.

@@ Coverage Diff @@
## develop #307 +/- ##
===========================================
+ Coverage 91.55% 91.58% +0.02% 
===========================================
 Files 49 49 
 Lines 2310 2318 +8 
===========================================
+ Hits 2115 2123 +8 
 Misses 195 195

Impacted Files	Coverage Δ
example/tests/integration/test_includes.py	`100% <ø> (ø)`	✅
example/serializers.py	`100% <100%> (ø)`	✅
.../tests/integration/test_non_paginated_responses.py	`100% <100%> (ø)`	✅
rest_framework_json_api/utils.py	`91.13% <100%> (+0.07%)`	✅
example/tests/integration/test_pagination.py	`100% <100%> (ø)`	✅

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dcf178...ede0ad4. Read the comment docs.

@mblayman mblayman mentioned this pull request

Dec 8, 2016

included_resources + ListSerializer #291

Closed


 Fix included resources M2M type (TODO test)

f0fa0e7

Copy link

Collaborator

mblayman commented Feb 20, 2017

@jerel hi! It looks like you are the current maintainer for DRF JA. Do you need some help with maintenance? The issue associated with this PR is killing my app's performance. I'd love to get it fixed and released, but I understand that things happen and people get swamped.

If I could lend a bit of a hand to help close some of this stuff out, I'd be happy to do that.

@jerel

Copy link

Member

jerel commented Feb 20, 2017

@mblayman I am; I would definitely appreciate help with maintenance.

Copy link

Collaborator

mblayman commented Feb 21, 2017

@rpatterson It seems like you've pushed some commits that are beyond the original scope of this PR. Is this stuff related? If so, can you explain? I think it would be difficult to review if this is a bunch of things all in one branch.

@rpatterson rpatterson force-pushed the develop branch from 6334ceb to f0fa0e7 Compare

February 22, 2017 17:14

Copy link

Contributor Author

rpatterson commented Feb 22, 2017

@mblayman Sorry, cleaned up my branches such that develop in my fork now only contains changes related to this PR, namely missing included resources.

mblayman

mblayman reviewed

rest_framework_json_api/utils.py

relation_model = relation.get_queryset().model

elif (

getattr(relation, 'many', False) and

hasattr(relation.child, 'Meta') and

Copy link

Collaborator

@mblayman mblayman Feb 22, 2017

Choose a reason for hiding this comment

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

I'm wondering if these hasattr checks are a bit too much. A serializer doesn't have to be a ModelSerializer so there is no requirement for Meta to exist, but isn't model a required attribute of Meta for a ModelSerializer? It feels slightly too defensive to me to check that model exists.

Copy link

Contributor Author

@rpatterson rpatterson Feb 22, 2017

Choose a reason for hiding this comment

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

I don't quite understand. If the serializer may not be a ModelSerializer, then shouldn't we not assume that Meta has a model?

Copy link

Collaborator

@mblayman mblayman Feb 22, 2017

Choose a reason for hiding this comment

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

Ah, you're right. I thought a vanilla serializer made no use of Meta, but I can see from the source that that is not correct. By all means, please keep model. 😊

mblayman

mblayman reviewed

rest_framework_json_api/utils.py Outdated

hasattr(relation.child.Meta, 'model')):

# For ManyToMany relationships, get the model from the child

# serializer of the list serializer

# TODO Test coverage

Copy link

Collaborator

@mblayman mblayman Feb 22, 2017

Choose a reason for hiding this comment

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

Could you either add a test to cover this line or delete the TODO comment? A test would be preferable, but understand if you can't. I don't think that TODO comments should be allowed in a master branch. Experience has showed me that they almost always languish and are forgotten until someone comes by later and deletes the comment.

Copy link

Contributor Author

@rpatterson rpatterson Feb 22, 2017

Choose a reason for hiding this comment

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

Removed the TODO comment

mblayman

mblayman reviewed

rest_framework_json_api/utils.py Outdated

meta = getattr(serializer.child, 'JSONAPIMeta', None)

try:

return list(serializer.JSONAPIMeta.included_resources)

return list(meta.included_resources)

Copy link

Collaborator

@mblayman mblayman Feb 22, 2017

Choose a reason for hiding this comment

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

Slightly contrary to what I stated up above, what do you think of changing this logic to have a hasattr check here? I'd guess that many (most?) serializers will not have included_resources. Relying on an AttributeError for normal flow control of the majority case is bound to be slow. I recognize this would slightly increase the scope of this branch. Would you be ok fixing that while you're in here?

Copy link

Contributor Author

@rpatterson rpatterson Feb 22, 2017

Choose a reason for hiding this comment

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

Done

Copy link

Collaborator

mblayman commented Feb 22, 2017

I'm excited to get this one fixed for my selfish interest of making my app not hammer my API with requests. I had a couple of minor comments that I think would be worth addressing.

I'm happy to merge once they've been considered. 👍 Thanks, @rpatterson!

rpatterson added 2 commits

February 22, 2017 11:49


 PR django-json-api#307 : Remove TODO comment

c4513e7


 PR django-json-api#307 : Fix included_resources assumption

a4e71d5

Copy link

Contributor Author

rpatterson commented Feb 22, 2017

@mblayman I think I addressed all your feedback. Let me know if I missed anything.

mblayman

mblayman reviewed

rest_framework_json_api/utils.py Outdated

meta = getattr(serializer.child, 'JSONAPIMeta', None)

try:

return list(serializer.JSONAPIMeta.included_resources)

return list(getattr(meta, 'included_resources', []))

Copy link

Collaborator

@mblayman mblayman Feb 22, 2017

Choose a reason for hiding this comment

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

I think the surrounding try/except is no longer needed, right?

Copy link

Contributor Author

@rpatterson rpatterson Feb 22, 2017

Choose a reason for hiding this comment

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

Doh! Done.


 PR django-json-api#307 : Fix redundant try/except

ede0ad4

Copy link

Collaborator

mblayman commented Feb 22, 2017

Dude, you rock! I'll merge it once CI goes green. Thanks again!

@mblayman mblayman merged commit a19f445 into django-json-api:develop