Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#23088 closed Cleanup/optimization (fixed)

Paginator.page_range is of inconsistent type in Py2/Py3

Reported by: Keryn Knight <django@…> Owned by: mardini
Component: Core (Other) Version: dev
Severity: Normal Keywords:
Cc: cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, Paginator.page_range uses the implementation native version of range, which under Python 2.x yields a list, while on 3.x it is a class of type range, largely analogous to 2.x's xrange

six is already imported into the module for Page.__getitem__ and so could be leveraged to at least be consistent.

In the pathological case, the python 2.x version could be considered problematic if yielding a large range, because the list is built up immediately in memory, where the 3.x version uses the efficient iterator.
I actually encountered this by passing a page_range in a template's context, which then gets copied into every child context (eg: via {% include %}) ... and gets worse when you try and figure out what happened by enabling debug-toolbar's context peeking panel (commit where I handled it by basically removing a 65,000 item list)

Change History (14)

comment:1 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted

comment:2 by mardini, 10 years ago

Owner: changed from nobody to mardini
Status: newassigned

comment:4 by Moayad Mardini <moayad.m@…>, 10 years ago

Resolution: fixed
Status: assignedclosed

In 6508db2ff9fc5be95fec903b3fa7ab8204fe316a:

Fixed #23088 -- Used six range type in Paginator.page_range.

comment:5 by Tim Graham <timograham@…>, 10 years ago

In ce95ab8f025cc9f35990f0c2d9a290eec1ece753:

[1.7.x] Fixed #23088 -- Used six range type in Paginator.page_range.

Backport of 6508db2ff9 from master

comment:6 by Florian Apolloner, 10 years ago

This might cause problems if someone iterates over page_range twice; eg to show a list of pages above and below the content. So this change has the possibility to break existing code; not sure what the best way to go ahead is -- one option would be to also force a list on python3 and tell ddt to get smarter :þ

comment:7 by mardini, 10 years ago

Hi apollo13,

Thanks for the feedback. I actually tried a small example in which I used the new version of page_range twice and didn't face any issues.
I think the reason for this is that although xrange(Python2)/range(Python3) is a generator in theory, the actual implementation is not.
A couple of links that discuss this behavior:
http://stackoverflow.com/questions/10776250/why-is-xrange-able-to-go-back-to-beginning-in-python
http://stackoverflow.com/questions/13092267/if-range-is-a-generator-in-python-3-3-why-can-i-not-call-next-on-a-range

comment:8 by Collin Anderson, 10 years ago

I've been relying on this being a list. #23140

Version 0, edited 10 years ago by Collin Anderson (next)

comment:9 by Collin Anderson, 10 years ago

Cc: cmawebsite@… added

comment:10 by Tim Graham <timograham@…>, 10 years ago

In ee0208108bd66cec04b28aca76eb506d9a2e1fd3:

Revert "[1.7.x] Fixed #23088 -- Used six range type in Paginator.page_range."

This reverts commit ce95ab8f025cc9f35990f0c2d9a290eec1ece753.

It caused some backwards compatibility concerns (refs #23140).

comment:11 by Florian Apolloner <florian@…>, 10 years ago

In 40fb6a560139150daa8f231f65b4044d45a97a8d:

Revert "Fixed #23088 -- Used six range type in Paginator.page_range."

This reverts commit 6508db2ff9fc5be95fec903b3fa7ab8204fe316a.

Refs #23088.

comment:12 by Florian Apolloner <florian@…>, 10 years ago

In 2d542bf60cc37c59872c5a20d6af3bf826038739:

Ensured that Paginator.page_range works the same on Python 2 and 3.

This somewhat fixes #23088, refs 23140.

comment:13 by Florian Apolloner <florian@…>, 10 years ago

In 1cb919e408d943cf2fadebb2b12517f523feb718:

[1.7.x] Ensured that Paginator.page_range works the same on Python 2 and 3.

This somewhat fixes #23088, refs 23140.

Backport of 2d542bf60cc37c59872c5a20d6af3bf826038739 from master.

comment:14 by Collin Anderson, 10 years ago

this ticket is closed as "fixed", so here's a new ticket for converting to an iterator #23190

Note: See TracTickets for help on using tickets.
Back to Top