Paginate decorator for arbitrary list-like objects

[lilspikey]

I've just been examining the code for the paginate decorator to see
whether it can handle more than just result sets and lists and was
slightly concerned to see that it does an isinstance(vardata,list)
rather than hasattr(vardata,'__len__') before calling len(vardata) to
get the number of items in the data being paginated. The code in
question being:

194-197 of http://trac.turbogears.org/browser/branches/1.0/turbogears/paginate.py

Now I presume this means that if I was to create an object that
implements __len__ and supports slicing that it won't work with the
paginate decorator? Or am I missing something?

I'd like to be able to create a custom "query" object so I can "fix"
this page:

http://chrss.co.uk/games/

Which shows a list of games and includes the number of moves in each
game. It's only broken in so far as at the moment I am performing one
query per game to get the number of moves. (I'm using SQLObject)

Ideally I'd like to be able to use some of the stuff I've played
around with here:

http://psychicorigami.com/2007/12/16/using-raw-sql-with-sqlobject-and-keeping-the-object-y-goodness/

To reduce that down to one query, whilst still using the paginate
decorator.

So can I use "list-like" objects with the paginate decorator? If not
would it be worth me submitting a patch for this?

cheers,
John

[Roger Demetrescu]

Hey John,

On Jan 4, 2008 9:36 AM, lilspikey <lils...@gmail.com> wrote:
>
> I've just been examining the code for the paginate decorator to see
> whether it can handle more than just result sets and lists and was
> slightly concerned to see that it does an isinstance(vardata,list)
> rather than hasattr(vardata,'__len__') before calling len(vardata) to
> get the number of items in the data being paginated. The code in
> question being:
>
> 194-197 of http://trac.turbogears.org/browser/branches/1.0/turbogears/paginate.py
>
> Now I presume this means that if I was to create an object that
> implements __len__ and supports slicing that it won't work with the
> paginate decorator? Or am I missing something?

Your concerns are reasonable. Indeed I believe we could do your proposal change.
This would also allow us to get rid of this check:

"""or (sqlalchemy and isinstance(var_data,
sqlalchemy.orm.attributes.InstrumentedList)"""

InstrumentedList is already a list subclass in SA 0.4. And in SA 0.3
it is a class which has
__len__() and slices methods.

So, lines 194-195 could be drastically reduced to a simple "if
hasattr(vardata,'__len__'):", as
you suggested.

<<SNIP>>

> So can I use "list-like" objects with the paginate decorator? If not
> would it be worth me submitting a patch for this?

A patch with tests would be very welcomed. Please don't forget to also
open a ticket for this.

Regarding the tests, you can write them in [1], which I hope it is not
complicated to understand... :)

[1] - http://trac.turbogears.org/browser/branches/1.0/turbogears/tests/test_paginate.py

Thanks !

> cheers,
> John

Just for future references, what is your last name ? :)

Cheers,

Roger

[Paul Johnston]

Hi,

slightly concerned to see that it does an isinstance(vardata,list)
rather than hasattr(vardata,'__len__') before calling len(vardata) to

This is probably because calling len on database result sets could result in an expensive COUNT(*) query, that you don't want paginate causing unnecessarily.

So, by all means propose a patch, but bear in mind this requirement.

Paul

[Diez B. Roggisch]

Paul Johnston schrieb:

The code needs the length of the overall result, how else are you going
to calculate the number of total pages? And the SO/SA result objects
should cretainly cache that value once it is computed by the DB - if
not, their broken.

so I don't see any problems here.

Diez

[lilspikey]

Hi Roger,
it might take me a while to get a patch together (never
actually done one before) and I'm currently still actually running TG
1.0... I'll open a ticket though and lay out what I said here and
hopefully that will motivate me to get a patch (with tests) submitted.
In the short-term I guess the only option for using a
list-like object would be to make sure it actually extends list I
guess? Just mentioning that for my own sake and for anyone else who
might be looking for a similar solution, but doesn't want to wait for
a patch.

cheers,
John Montgomery

p.s. real noob question, but is there a could place to start reading
up about how one actually makes a proper patch? I'm guessing it's
basically just a diff between my code and the last version in svn, but
as I said I've never tried this before.

On Jan 4, 12:35 pm, "Roger Demetrescu" <roger.demetre...@gmail.com>
wrote:
> Hey John,

>
> On Jan 4, 2008 9:36 AM, lilspikey <lilspi...@gmail.com> wrote:
>
>
>
> > I've just been examining the code for the paginate decorator to see
> > whether it can handle more than just result sets and lists and was
> > slightly concerned to see that it does an isinstance(vardata,list)
> > rather than hasattr(vardata,'__len__') before calling len(vardata) to
> > get the number of items in the data being paginated.  The code in
> > question being:
>

> > 194-197 ofhttp://trac.turbogears.org/browser/branches/1.0/turbogears/paginate.py

>
> > Now I presume this means that if I was to create an object that
> > implements __len__ and supports slicing that it won't work with the
> > paginate decorator?  Or am I missing something?
>
> Your concerns are reasonable. Indeed I believe we could do your proposal change.
> This would also allow us to get rid of this check:
>
> """or (sqlalchemy and isinstance(var_data,
> sqlalchemy.orm.attributes.InstrumentedList)"""
>
> InstrumentedList is already a list subclass in SA 0.4. And in SA 0.3
> it is a class which has
> __len__() and slices methods.
>
> So, lines 194-195 could be drastically reduced to a simple "if
> hasattr(vardata,'__len__'):", as
> you suggested.
>
> <<SNIP>>
>
> > So can I use "list-like" objects with the paginate decorator?  If not
> > would it be worth me submitting a patch for this?
>
> A patch with tests would be very welcomed. Please don't forget to also
> open a ticket for this.
>
> Regarding the tests, you can write them in [1], which I hope it is not
> complicated to understand...  :)
>

> [1] -http://trac.turbogears.org/browser/branches/1.0/turbogears/tests/test...

[Christopher Arndt]

lilspikey schrieb:

> p.s. real noob question, but is there a could place to start reading
> up about how one actually makes a proper patch? I'm guessing it's
> basically just a diff between my code and the last version in svn, but
> as I said I've never tried this before.

Good thing you asked. Actually, we prefer that you use "svn diff" on
your own SVN checkout. It's all explained here:

http://docs.turbogears.org/Contributing

and here:

http://docs.turbogears.org/patching_guidelines

HTH, Chris

[Roger Demetrescu]

On Jan 4, 2008 4:52 PM, lilspikey <lils...@gmail.com> wrote:
>
> Hi Roger,
> it might take me a while to get a patch together (never
> actually done one before) and I'm currently still actually running TG
> 1.0... I'll open a ticket though and lay out what I said here and
> hopefully that will motivate me to get a patch (with tests) submitted.
> In the short-term I guess the only option for using a
> list-like object would be to make sure it actually extends list I
> guess? Just mentioning that for my own sake and for anyone else who
> might be looking for a similar solution, but doesn't want to wait for
> a patch.

Without some black-magic [1], yes... you will need to extend `list` class.

> cheers,
> John Montgomery
>
> p.s. real noob question, but is there a could place to start reading
> up about how one actually makes a proper patch? I'm guessing it's
> basically just a diff between my code and the last version in svn, but
> as I said I've never tried this before.

I have always use TortoiseSVN "Create patch" command (that's for Windows).
If you are running svn from command line (eg: linux), I believe the command is:

$ svn diff > my_patch.patch

If I am wrong, someone correct me, please...

[]s
Roger

[1] - By "black magic", I mean doing something like this:

----- python shell -----------

>>> class A(object): pass

>>> class B(A): pass

>>> class C(object): pass

>>> c = C()
>>> isinstance(c, A)
False
>>> isinstance(c, C)
True
>>> c.__class__
<class '__main__.C'>
>>> c.__class__ = B # changing the class of this instance
>>> isinstance(c, C)
False
>>> isinstance(c, A)
True

----- python shell / -----------

Note that this won't work with "list". If you try it, you will get an:

=======
Traceback (most recent call last):
File "<pyshell#31>", line 1, in <module>
c.__class__ = int
TypeError: __class__ assignment: only for heap types
=======

[lilspikey]

Hi Roger,
I've submitted the ticket: http://trac.turbogears.org/ticket/1648

I left everything on the defaults, so I guess it will
need to be looked at and changed.

I'll have a go at creating a patch, but I guess it
wouldn't be too hard for someone else to take on if that'd be
quicker. I've got an old windows laptop that I can don't use for TG
stuff, so I might use that for the patch - rather than risk messing up
my dev machine setup for now.

cheers,
John

On Jan 4, 7:29 pm, "Roger Demetrescu" <roger.demetre...@gmail.com>
wrote:

[Roger Demetrescu]

On Jan 4, 2008 6:05 PM, lilspikey <lils...@gmail.com> wrote:
>
> Hi Roger,
> I've submitted the ticket: http://trac.turbogears.org/ticket/1648

I took a look at it. Well done !! :D

> I left everything on the defaults, so I guess it will
> need to be looked at and changed.

Ok...

> I'll have a go at creating a patch, but I guess it
> wouldn't be too hard for someone else to take on if that'd be
> quicker. I've got an old windows laptop that I can don't use for TG
> stuff, so I might use that for the patch - rather than risk messing up
> my dev machine setup for now.

Cool !! You'll see that, once you start coding patches, it's hard to stop... ;)

[]s
Roger