Paginate: sorting by multiple fields in default_order

@paginate("company", default_order="company.name")

I'm using paginate in an application where the screen should start out ordered
by multiple fields. For example, a list of businesses sorted by state/province,
city, and business name. It's currently possible to arrive at the correct
ordering by clicking on the column heading in the right sequence, but I'd
really like to have the screen start out in the right order.

What my application needs is the ability to do this:

@paginate("company", default_order="company.state,company.city,company.name")

or this:

@paginate("company", default_order="company.region,-company.billing_date")

(The dash before the second field indicates that it should be sorted in reverse
order.)

Actually, I've already modified my own working copy of paginate.py to do this,
and it seems to be working quite well. Here are my questions:

1. Would this be useful to anyone else?

2. If so, does this seem like a reasonable new definition of default_order?
Old definition: "a single field name"
New definition: "a single field name or a comma separated list of field
names, where each field name is optionally preceded by a dash (-) to indicate
reverse order"

The ability to reverse the ordering from within "default_order" makes the
"default_reversed" parameter effectively unnecessary. However, for the sake of
compatibility I left it in, and made sure that all combinations of
default_reversed (True or False) and having a leading dash (or not) in
default_order work as expected.

As far as I can tell, this change should be fully backwards-compatible.

Any comments or suggestions?

- Joel
--
Joel Pearson

I'd rather have a list here:
default_order=['company.region', '-company.billing_date']

Using a list for more than one item yes.

As I said, I prefer using a list to mean a list and a string when referring to
a string. A simple "isinstance(default_order, basestring)" would be able to
differentiate which is which.

Did you also made it a list? Because if we have more than one value, then
default_reversed can have different behaviours for each member of the list...

Or it should check if there's just one "order" column, while raising an
exception if you have multiple columns.

--
Jorge Godoy <jgo...@gmail.com>

Definitely you own a mind reader machine.... :)

I was talking about this kind of feature 2 weeks ago, with my team... :)

+1 on using a list... it make things cleaner.

My only doubt is: how does default_reversed interfere in this new kind
of expression ?

I mean:

default_order=['company.region', '-company.billing_date'], default_reversed=True

should it be understand as:

default_order=['-company.region', 'company.billing_date'] ?

Maybe we could use the "-" prefix to specify reverse order even for
the case when default_order
is a base string, and start deprecating default_reversed.

(default_order="foobar", default_reversed=True) ==
(default_order="-foobar") with a deprecate warning

(default_order=["foo", "-bar"], default_reversed=True) ==
Exception("you should no use default_reversed when default_order is a
list")

+1

God ! Why didn't I read all the message before typing my reply ??
It would prevent me to write all those lines above... :)

[]s
Roger

PS: I didn't forget the poll about paginate. I just need to find some
time to implement the proposed ideas.

I agree that a list is clearer; I used a string to avoid changing the data type
of default_order.

I see what you're saying-- allow default_order to be either:
A) a string containing a single field name, or
B) a list of strings that each contain a single field name
and then use "isinstance" to process each case accordingly.

Good idea. I've updated my code to reflect that change.

Actually, I was thinking that default_reversed could be eliminated entirely in
some future release (like 1.1 or 2.0, not 1.0.4).

I prefer these:

@paginate('table', default_order='-field1')
@paginate('table', default_order=['field1', '-field2'])

to these:

@paginate('table', default_order='field1', default_reversed=True)
@paginate('table', default_order=['field1', 'field2'],
default_reversed=[False, True])

If default_reversed was going away in a future release, it could remain a
single True/False value that works just the way it always has, by reversing the
sort order of the first (or only) field in default_order. This maintains
compatibility with existing applications (which only have a single field in
default_order), while allowing complex sort orders (using a leading dash when
needed) for new applications.

The only use case I can think of for expanding default_reversed into a list is
that it is somewhat easier to programmatically toggle the True/False values in
a list structure (default_reversed) than it is to add and remove the leading
dashes in a list of strings (default_order). The price you pay for having "more
than one way to do it", however, is the potential for confusing application
code like this:

# what's the actual order?
@paginate('table', default_order=['-field1', '-field2', 'field3'],
default_reversed=[False, True, True])

along with the additional logic in paginate.py to calculate the actual sort
order using both default_reversed and dashes in default_order.

All things considered, I'd prefer to (eventually) phase out default_reversed,
rather than expand it, but I welcome other opinions. :-)

Me too. :-)

It could also be that:

if default_order[n].startswith('-'):
ignore_default_reversed() # ;-)

I.e., we don't allow cascading the two different options.

If default_reversed is going away, deprecated messages should be issued as
soon as possible so that people have time to update their code.

+1 from me, with deprecated messages.

--
Jorge Godoy <jgo...@gmail.com>

<SNIP>

Have you any news on this ?
I'd really like to see this feature in TG 1.0.4.... ;)

If you can quickly fill a ticket with your patch, we can ask Florent
to wait a couple of hours more before releasing 1.0.4b3 ...

And don't worry about tests. I can code them before applying your patch... :)

[]s
Roger

Sorry for the delay. I just went to re-create the patch against the current
version, but I won't be able to finish merging my changes for another 6 hours
or so, due to prior commitments. Also, I didn't mention previously that my
current version alters the appearance of the URL variable *_tgp_ordering from
being a URL-encoded dict to being a simple comma-separated list of field names
in sort order, with optional leading dashes on each field name to indicate
reversed order. (Note: I didn't change any internal data structures, just how
the URL is produced in get_href() and how it is parsed in convert_order().)

Thus, what previously looked like this (in the URL):

company_tgp_ordering=%7B%27state%27%3A%20%5B1%2C%20True%5D%2C%20u%27city%27%3A%20%5B0%2C%20True%5D%7D

now looks like this:

company_tgp_ordering=state,city

This would break any existing bookmarks, but it would be easy enough to add
back in code to detect the old format, if desired. For that matter, I could
just throw out that part of the code and use the old format.

I'll open the ticket once the patch is ready. Let me know what to do about the
_tgp_ordering variable. Thanks.

Don't worry... I got some time free and started coding the patch myself.

It is finished and all tests are running ok.

I am just polishing the tests and comments, and will commit them very
soon... (in 1/2 hour)

If you didn't create a ticket already, I'll do it, linking it to this thread.

Thanks for motivating us !! :D

[]s
Roger

Hmm.. the actual code already produces an ugly URL when you start
changing the order of columns, so I think this is not an issue.

Basically the core portion of my modification became:

====
elif default_order and not ordering:
# ordering = {default_order:[0, not default_reversed]}

if isinstance(default_order, basestring):
# adapt old style to new style
df = [(default_reversed and "-" or "") + default_order]
elif default_reversed:
raise StandardError("default_reversed (deprecated) is only "
" allowed when default_order is basestring type")
else:
df = default_order

ordering = dict([(v.lstrip('-'), [k, not v.startswith('-')])
for k,v in enumerate(df)])
====

BTW, It doesn't handle some strange configuration, like ['city',
'----street'], for the sake of simplicity...

I hope it will work as expected... :)

[]s

Done at [1] and [2]. :)

[1] - http://trac.turbogears.org/changeset/3744
[1] - http://trac.turbogears.org/changeset/3745

Good work, Roger!

I just finished reviewing your changes. They look good to me, and they're more
concise than what I had written, as well. I had attempted to handle the
unlikely case where default_order is a list and default_reversed is also used--
an unpleasant mix of the old and new strategies that I hoped no one would ever
use. But now that default_reversed is officially deprecated, it makes sense to
simply return an error in such cases, as your code does.

I also updated my TG checkout, and the new code works as expected with my
existing application.

Thanks for getting this done. It's especially helpful for me, since I already
have an app in production that depends on this feature! :-)

Thanks Roger!

I'll release 1.0.4b3 tonight then... and this time we freeze the
functionalities for _real_.
And don't try to lobby more features into 1.0 :-)

Thanks for the time and implication.
Florent.

Great !! That is good news. :D

You are welcome.

[]s
Roger

Well, *I* thank you for not releasing 1.0.4b3 yesterday... :)

Hahahaha... ok... I promise I won't do it. :)

[]s
Roger