[Bug 1399639] Updating Django Rest Framework #4664
Conversation
requirements/default.txt
Outdated
| --hash=sha256:4962418a57804f8323282728a4f9b9496e78caec1adda352170697752eff01bf | ||
| djangorestframework==3.5.4 \ | ||
| --hash=sha256:110afa12784ceadfb50808882689302d266785b51e3d13286744333ff6d78e60 \ | ||
| --hash=sha256:f995a35ae22f354d2a9a42ee6d2c059c101f826b1485ed46781677895ad25ee5 |
There was a problem hiding this comment.
I'm adding more links to the requirements files, see https://github.com/mozilla/kuma/blob/master/requirements/README.rst#requirements-format. Here's what I have:
# RESTful API framework, used by search
# Code: https://github.com/encode/django-rest-framework
# Changes: http://www.django-rest-framework.org/topics/release-notes/
# Docs: http://www.django-rest-framework.org
djangorestframework==3.5.4 \
--hash=sha256:110afa12784ceadfb50808882689302d266785b51e3d13286744333ff6d78e60 \
--hash=sha256:f995a35ae22f354d2a9a42ee6d2c059c101f826b1485ed46781677895ad25ee5
There was a problem hiding this comment.
I've tested DRF 3.6.4, and that works with these changes as well. Please switch to that version.
|
Thanks for tackling this @safwanrahman! I appreciate that you are limiting changes to those needed for the upgrade. This is quick feedback, I'll get into a detailed review later today. Tests passing is a good sign, although I really don't trust the tests around search 😬 . I'll read the release notes and poke around to see if these look good. I'm taking an incremental approach with these updates, shipping one or two packages at a time. I think DRF 3.6 supports Django 1.8 still, and if so, we should get to that eventually. I'm happy to focus on DRF 3.5, then do DRF 3.6 in the next PR, if there are a lot of changes for DRF 3.6. |
jwhitlock
left a comment
There was a problem hiding this comment.
I manually tested this, and it works for me locally. The changes from DRF 3.3 to 3.6 are mostly around core API and the new schema tools, which we aren't using yet.
Please add the URLs to the requirements file, and update to DRF 3.6.4. I'd like to merge and deploy this on Tuesday, Feb 20.
requirements/default.txt
Outdated
| --hash=sha256:4962418a57804f8323282728a4f9b9496e78caec1adda352170697752eff01bf | ||
| djangorestframework==3.5.4 \ | ||
| --hash=sha256:110afa12784ceadfb50808882689302d266785b51e3d13286744333ff6d78e60 \ | ||
| --hash=sha256:f995a35ae22f354d2a9a42ee6d2c059c101f826b1485ed46781677895ad25ee5 |
There was a problem hiding this comment.
I've tested DRF 3.6.4, and that works with these changes as well. Please switch to that version.
|
|
||
| context = self.resolve_context(data, request, response) | ||
| return template.render(context, request=request) | ||
| return super(ExtendedTemplateHTMLRenderer, self).get_template_context(data, renderer_context) |
There was a problem hiding this comment.
😄 good job resolving this TODO, the code is much cleaner.
Codecov Report
@@ Coverage Diff @@
## master #4664 +/- ##
==========================================
- Coverage 95.27% 95.27% -0.01%
==========================================
Files 261 261
Lines 23571 23560 -11
Branches 1692 1691 -1
==========================================
- Hits 22458 22447 -11
Misses 902 902
Partials 211 211
Continue to review full report at Codecov.
|
|
@jwhitlock Updated with the comments. |
jwhitlock
left a comment
There was a problem hiding this comment.
Thanks @safwanrahman - one step closer to the current DRF!
It passes all the tests! But need some more investigation.
I will investigate more later this week.
@jwhitlock Can you take a look if I have taken proper approach?