Opened 4 years ago

Closed 4 years ago

Last modified 4 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: master
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 Changed 4 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:2 Changed 4 years ago by mardini

Owner: changed from nobody to mardini
Status: newassigned

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

Resolution: fixed
Status: assignedclosed

In 6508db2ff9fc5be95fec903b3fa7ab8204fe316a:

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

comment:5 Changed 4 years ago by Tim Graham <timograham@…>

In ce95ab8f025cc9f35990f0c2d9a290eec1ece753:

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

Backport of 6508db2ff9 from master

comment:6 Changed 4 years ago by Florian Apolloner

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 Changed 4 years ago by mardini

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 Changed 4 years ago by Collin Anderson

I've been relying on this being a list, so it would be nice if it stayed that way or we should document that we changed it on python2. #23140

Last edited 4 years ago by Collin Anderson (previous) (diff)

comment:9 Changed 4 years ago by Collin Anderson

Cc: cmawebsite@… added

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

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 Changed 4 years ago by Florian Apolloner <florian@…>

In 40fb6a560139150daa8f231f65b4044d45a97a8d:

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

This reverts commit 6508db2ff9fc5be95fec903b3fa7ab8204fe316a.

Refs #23088.

comment:12 Changed 4 years ago by Florian Apolloner <florian@…>

In 2d542bf60cc37c59872c5a20d6af3bf826038739:

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

This somewhat fixes #23088, refs 23140.

comment:13 Changed 4 years ago by Florian Apolloner <florian@…>

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 Changed 4 years ago by Collin Anderson

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