Edgewall Software
Modify

Opened 10 years ago

Last modified 3 years ago

#10178 new enhancement

[PATCH] Allow TracQueries to order on multiple columns

Reported by: david@… Owned by:
Priority: high Milestone: next-major-releases
Component: query system Version: 0.12dev
Severity: normal Keywords: patch
Cc: macke@…, andrew.c.martin@…, jcampbell@…, azoff@…, osimons, fabien.weyh@…, ethan.jucovy@…, phep-lists@…, phep-lists@… Branch:
Release Notes:

ticket: Added the possibility to sort on multiple columns in the [[TicketQuery]] macro.

API Changes:
Internal Changes:

Description (last modified by Remy Blank)

Currently trac only supports ordering on a single column. I've written a patch which provides the ability to sort on more than one column.

This allows:

[[TicketQuery(max=5,order=priority|somecolumn, desc=0|1, status=open)]]

Attachments (5)

trac_ordering.patch (6.8 KB ) - added by david@… 10 years ago.
Add multiple column ordering to trac queries
trac_ordering.2.patch (6.8 KB ) - added by david@… 10 years ago.
Updated patch
trac_ordering-r2.patch (6.5 KB ) - added by david@… 10 years ago.
Second revision
trac_ordering.3.patch (6.0 KB ) - added by david@… 10 years ago.
3rd Revision
10178-multi-column-order-r10691.patch (7.0 KB ) - added by Remy Blank 10 years ago.
Fixed and simplified.

Download all attachments as: .zip

Change History (34)

by david@…, 10 years ago

Attachment: trac_ordering.patch added

Add multiple column ordering to trac queries

comment:1 by david@…, 10 years ago

Summary: Allow TracQueries to order on multiple columns[PATCH] Allow TracQueries to order on multiple columns

comment:2 by Remy Blank, 10 years ago

Component: generalquery system
Description: modified (diff)
Owner: set to Remy Blank
Priority: normalhigh
Release Notes: modified (diff)

Looks very nice, thank you!

comment:3 by Christian Boos, 10 years ago

Some suggestions on the patch:

  • type(order) != type([]) should be not isinstance(order, list)
  • if order is not None and ...: order = [order] but later [... for o in order] which will fail if order is actually None
  • desc = ...
    • 80 cols limit (TracDev/CodingStyle)
    • self.order > self.desc is likely not doing what you think it does (e.g. ['a'] > [0] is True)
  • tabs near the end of the patch?

by david@…, 10 years ago

Attachment: trac_ordering.2.patch added

Updated patch

comment:4 by david@…, 10 years ago

OK, I think I've covered the suggested changes and attached the modified patch.

comment:5 by Christian Boos, 10 years ago

Well, it seems you uploaded the same trac_ordering.patch again…

comment:6 by david@…, 10 years ago

Argh. How did I manage that? Anyway…I swear this is the correct patch this time.

by david@…, 10 years ago

Attachment: trac_ordering-r2.patch added

Second revision

comment:7 by david@…, 10 years ago

Well, I noticed that I had moved as_str() unnecessarily, so rev 3 of this patch just drops the movement of that function. Otherwise it is the same as rev 2.

by david@…, 10 years ago

Attachment: trac_ordering.3.patch added

3rd Revision

comment:8 by Remy Blank, 10 years ago

trac_ordering.3.patch makes 4 test cases fail. I have simplified and fixed the patch in 10178-multi-column-order-r10691.patch, but I have a bad feeling about the change. More specifically, treating the order and desc arguments separately feels wrong, and using a single [(field, desc), ...] list as an order argument would feel more natural. Except for the case where order isn't specified, but desc is, which means to use the default ordering but descending.

Also, the fix in get_default_columns() is currently backward-compatible, but not necessarily the best solution.

Ideas and review welcome.

by Remy Blank, 10 years ago

Fixed and simplified.

comment:9 by Marcus Lindblom Sonestedt <macke@…>, 9 years ago

Cc: macke@… added

comment:10 by Marcus Lindblom Sonestedt <macke@…>, 9 years ago

How about just adding a ! or ~ prefix to the order fields to indicate that they should be reversed, i.e. something like:

[[TicketQuery(order=priority|~created|resolution)]]

I think the tilde (~) is easier to distinguish from a pipe than the exclamation mark.

Underscore or just a dash are other options.

comment:11 by Andrew C. Martin <andrew.c.martin@…>, 9 years ago

Cc: andrew.c.martin@… added

+1

comment:12 by Christian Boos, 9 years ago

Good idea, though - is much more intuitive in this situation than ~.

comment:13 by Remy Blank, 9 years ago

Sounds good. We can keep desc for backward compatibility, and it would only affect the first element in the order list (i.e. for the first element in order, sorting is descending iff the element starts with "-" or desc=1).

comment:14 by osimons, 9 years ago

Agree. Splitting order and desc is not practical. As cboos said, using minus (order=priority|-created|resolution) is what seems most intuitive for negating the order effect.

Decisions in this ticket also has some bearing on how [query] default_order is implemented in #10425.

comment:15 by jcampbell@…, 9 years ago

Cc: jcampbell@… added

comment:16 by Torbjörn Svensson <azoff@…>, 9 years ago

Cc: azoff@… added

comment:17 by osimons, 9 years ago

Cc: osimons added

comment:18 by Torbjörn Svensson <azoff@…>, 9 years ago

Multi-order has been integrated in patch for #10425 as requested by osimons.

Remaining work on this issue:

  • Define a way for the user to select what columns to order by and if the order is to be reversed.
  • Remove limitation in #10425 patch for a single rule (Query.init())
  • Enable the test case for multi order.

comment:19 by fabien.weyh@…, 9 years ago

Cc: fabien.weyh@… added

comment:20 by Remy Blank, 9 years ago

Milestone: 1.0next-major-0.1X

Moving unfinished stuff post-1.0.

comment:21 by Craig Silver <notify_craig@…>, 8 years ago

I am new to Trac so maybe I have my versioning mixed up but mine says it's 1.0.1. Does that mean this ticket should already be implemented and released? I tried this in my TracQuery but it doesn't sort by the 2nd field:

order=priority|modified

comment:22 by osimons, 8 years ago

No, 'version' applies to version used when creating the ticket (or where bug is discovered, or as a marker of similar use). This ticket is still open, scheduled for some future non-specific milestone. To be available for you, a ticket should be 'closed' as 'fixed' and with a 'milestone' earlier or equal to the version you are using.

comment:23 by ethan.jucovy@…, 8 years ago

Cc: ethan.jucovy@… added

comment:24 by Craig Silver <notify_craig@…>, 8 years ago

Thanks for clarifying, osimons. I'm surprised that multi-column sorting is not yet supported. No estimate on its release, I guess?

comment:25 by Ryan J Ollos, 6 years ago

Owner: Remy Blank removed

comment:26 by Patrice Pillot <phep-lists@…>, 6 years ago

Cc: phep-lists@… added

comment:27 by figaro, 5 years ago

Keywords: patch added

comment:28 by anonymous, 5 years ago

Is there a reason that this hasn't been included in a release 4 years after it was completed?

comment:29 by Patrice Pillot <phep-lists@…>, 3 years ago

Cc: phep-lists@… added

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.