Fix for apps that don't load contenttypes module.#345

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

Jump to bottom

Merged

mblayman merged 1 commit intodjango-json-api:developfromamw:fix-no-contenttypes

May 7, 2017

Merged

Fix for apps that don't load contenttypes module.#345

mblayman merged 1 commit intodjango-json-api:developfromamw:fix-no-contenttypes

May 7, 2017

Conversation

Copy link

Contributor

amw commentedMay 7, 2017

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.

I would like to add a test for this, but it would require two separate Django configs. Not sure how you would want to handle that in py.test.

Also usingtype(something) is object() comparison as a safer alternative totype(something) is type(None). Ifsomething happened to beNone we would enter a branch that was never supposed to run.

Copy link

codecov-io commentedMay 7, 2017•
edited
Loading

Codecov Report

Merging#345 intodevelop willincrease coverage by<.01%.
The diff coverage is80%.

@@             Coverage Diff             @@##           develop     #345      +/-   ##===========================================+ Coverage    76.37%   76.38%   +<.01%===========================================  Files           50       50                Lines         6024     6025       +1     ===========================================+ Hits          4601     4602       +1  Misses        1423     1423

Impacted Files	Coverage Δ
rest_framework_json_api/utils.py	`89.79% <80%> (+0.04%)`	⬆️

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updateda97874...20933e6. Read thecomment docs.

Copy link

Collaborator

mblayman commentedMay 7, 2017

I agree that a test would be nice, but this might be one of those cases where the setup for that test would be extreme overkill since it involves things that happen at import time.

What do you think of adding a# pragma: no cover to theif clause that checks for contenttypes inINSTALLED_APPS? It seems like that might be a clue to future readers that this was considered and declared "not worth testing."

Fix for apps that don't load contenttypes module.

20933e6

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.