Replace parse_header with parse_header_parameters #8554
Conversation
There was a problem hiding this comment.
Hey @jaap3 — thanks for this, looks good.
Just one initial comment on the compat change...
@shaib's comment from #8543 (comment) about WHY we decode() in the shim may be worth adding, for later comprehension too.
…tp.parse_header_parameters Add a backwards compatibility shim for Django versions that have no (or an incompatible) django.utils.http.parse_header_parameters implementation.
|
I've addressed the review comments. BTW, as this issue is only really present in django's main branch it's hard to spot the breakage in the CI, as tox is setup to allow failure for that particular "version". Looking at the results here https://github.com/encode/django-rest-framework/runs/7320233994?check_suite_focus=true#step:6:363 you'll see that djangomain now passes, when it didn't before (i.e. the most recent run for master: https://github.com/encode/django-rest-framework/runs/7094624536?check_suite_focus=true#step:6:363) |
| # which is slightly different from the one in 4.2, it needs | ||
| # the compatibility shim as well. | ||
| from django.utils.http import parse_header_parameters | ||
| parse_header_parameters = parse_header_parameters |
There was a problem hiding this comment.
This line isn't needed I think. 🤔
There was a problem hiding this comment.
flake8 yells at me if I don't do this
There was a problem hiding this comment.
Hmm. OK. Let me have a look... 🤔
Thanks!
Yes... We didn't yet get to failing on breaks against django/django main, but probably should at some point. //cc @tomchristie |
|
Hi @jaap3...
Hmmm 🤔 Not sure what's going on there. Passes locally for me. Also on CI https://github.com/encode/django-rest-framework/runs/7339596012?check_suite_focus=true I'll finish this in #8556, since the Allow maintainers to make edits box seems to be unchecked. Thanks for the input. Great stuff! 🏅 |
Add a backwards compatibility shim for Django versions that have no (or an incompatible)
django.utils.http.parse_header_parameters implementation.
This is an alternative fix to the one proposed in #8543, which also attempts to fix #8540.
While making this PR I noticed that get_encoded_filename is not necesarry anymore as this is already handled correctly in Django ~1.8 (django/django@b42e5ca)