Edgewall Software
Modify

Opened 6 years ago

Last modified 11 months 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@…
Release Notes:

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

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

Download all attachments as: .zip

Change History (33)

Changed 6 years ago by david@…

Attachment: trac_ordering.patch added

Add multiple column ordering to trac queries

comment:1 Changed 6 years ago by david@…

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

comment:2 Changed 6 years ago by Remy Blank

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

Looks very nice, thank you!

comment:3 Changed 6 years ago by Christian Boos

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?

Changed 6 years ago by david@…

Attachment: trac_ordering.2.patch added

Updated patch

comment:4 Changed 6 years ago by david@…

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

comment:5 Changed 6 years ago by Christian Boos

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

comment:6 Changed 6 years ago by david@…

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

Changed 6 years ago by david@…

Attachment: trac_ordering-r2.patch added

Second revision

comment:7 Changed 6 years ago by david@…

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.

Changed 6 years ago by david@…

Attachment: trac_ordering.3.patch added

3rd Revision

comment:8 Changed 6 years ago by Remy Blank

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.

Changed 6 years ago by Remy Blank

Fixed and simplified.

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

Cc: macke@… added

comment:10 Changed 6 years ago by Marcus Lindblom Sonestedt <macke@…>

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 Changed 6 years ago by Andrew C. Martin <andrew.c.martin@…>

Cc: andrew.c.martin@… added

+1

comment:12 Changed 6 years ago by Christian Boos

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

comment:13 Changed 6 years ago by Remy Blank

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 Changed 6 years ago by osimons

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 Changed 6 years ago by jcampbell@…

Cc: jcampbell@… added

comment:16 Changed 5 years ago by Torbjörn Svensson <azoff@…>

Cc: azoff@… added

comment:17 Changed 5 years ago by osimons

Cc: osimons added

comment:18 Changed 5 years ago by Torbjörn Svensson <azoff@…>

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 Changed 5 years ago by fabien.weyh@…

Cc: fabien.weyh@… added

comment:20 Changed 5 years ago by Remy Blank

Milestone: 1.0next-major-0.1X

Moving unfinished stuff post-1.0.

comment:21 Changed 4 years ago by Craig Silver <notify_craig@…>

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

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 Changed 4 years ago by ethan.jucovy@…

Cc: ethan.jucovy@… added

comment:24 Changed 4 years ago by Craig Silver <notify_craig@…>

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

comment:25 Changed 2 years ago by Ryan J Ollos

Owner: Remy Blank deleted

comment:26 Changed 21 months ago by Patrice Pillot <phep-lists@…>

Cc: phep-lists@… added

comment:27 Changed 16 months ago by figaro

Keywords: patch added

comment:28 Changed 11 months ago by anonymous

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
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.