Fix qfactor rankings for HTTP-ACCEPT #340
Conversation
There was a problem hiding this comment.
Thanks a bunch! Just one minor naming suggestion for some extra clarity.
The Travis build is currently broken and waiting on #338. Once that is in, we can re-run Travis and get this moving on.
graphene_django/views.py
Outdated
| accepted_length = len(accepted) | ||
| #the list will be ordered in preferred first - so we have to make | ||
| #sure the most preferred gets the highest number | ||
| html_index = accepted_length - accepted.index('text/html') if 'text/html' in accepted else 0 |
There was a problem hiding this comment.
Yeah, count is definitely not accounting for priority here. Thanks for the fix.
Can we rename html_index and json_index to html_priority and json_priority? I was a little confused at first because their index wouldn't need accepted_length. But I now see why we need the accepted_length - accepted.index(... part to make the html_priority > json_priority condition work.
Thanks!
There was a problem hiding this comment.
Good Point - the names were leftovers from before the fixes - I have updated the names
|
@syrusakbary This PR looks good to me, and is ready for your approval/merge. |
Currently qfactor is being parsed - and they are ordered in order of preference - but when i comes time to check if to show html vs json it just checks if either of them exist and then show html if html exists in the list of accepted headers and json doesn't - instead of figuring out what's the first in the list.
Also - when splitting multiple accepted headers it leaves a space in front of the second one and on - so I've added a
.strip()Also added tests