Django 1.10 support. by tomchristie · Pull Request #4158 · encode/django-rest-framework (original) (raw)
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 our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation8 Commits14 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Working towards Django 1.10 support.
| @@ -635,6 +635,7 @@ def get_context(self, data, accepted_media_type, renderer_context): |
|---|
| 'view': view, |
| 'request': request, |
| 'response': response, |
| 'user': request.user, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed by Django 1.10 ?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For one reason or another, yes.
I've not dug into why yet, but presumably we were previously having it automatically included by the RequestContext. I assume that we could also change our settings in order to resolve this (eg perhaps context processor configuration has moved to being part of the TEMPLATES dictionary?)
In any case it's probably best that we pass it explicitly, rather than relying on the correct context processors being set.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified. Previously we were relying on the default value of TEMPLATE_CONTEXT_PROCESSORS, which includes django.contrib.auth.context_processors.auth.
We could add this in to the TEMPLATES.OPTIONS.context_processors in conftest.py, which would resolve our test cases, but it's better if we pass it explicitly, and not rely on the user settings.
Current coverage is 91.47%
@@ master #4158 diff @@
Files 51 51
Lines 5483 5487 +4
Methods 0 0
Messages 0 0
Branches 0 0
- Hits 5016 5019 +3
- Misses 467 468 +1
Partials 0 0
Powered by Codecov. Last updated by fe2aede...c65c5e4
There's an outstanding issue, downstream in django-filters, which uses get_field_by_name which does not exist across all versions. Once that's resolved we can remove the "expected failure" marker.
ERROR collecting tests/test_filters.py
=====================
tests/test_filters.py:40: in <module>
class SeveralFieldsFilter(django_filters.FilterSet):
env/lib/python2.7/site-packages/django_filters/filterset.py:197: in __new__
new_class.filter_for_reverse_field)
env/lib/python2.7/site-packages/django_filters/filterset.py:111: in filters_for_model
field = get_model_field(model, f)
env/lib/python2.7/site-packages/django_filters/filterset.py:93: in get_model_field
rel, model, direct, m2m = opts.get_field_by_name(parts[-1])
E AttributeError: 'Options' object has no attribute 'get_field_by_name'
In the meantime there's no reason not to merge this now, as it resolves almost all our issues, and the remaining failure is in the "expected failures" matrix.
👍 Let me know if you want some of my time, too.
Only if it's hard. :-) — It should be OK.
3 tasks
rlucioni pushed a commit to openedx/course-discovery that referenced this pull request
rlucioni pushed a commit to openedx/course-discovery that referenced this pull request
rlucioni pushed a commit to openedx/course-discovery that referenced this pull request
rlucioni pushed a commit to openedx/course-discovery that referenced this pull request