Excerpts from Rob Miller's message of Fri Jan 23 15:57:34 -0500 2009:
> Nicholas Bergson-Shilcock wrote:
> > Excerpts from Rob Miller's message of Fri Jan 23 13:30:36 -0500 2009:
> >> Nicholas Bergson-Shilcock wrote:
> >>> While troubleshooting a bug with the recently added next/previous thread links,
> >>> I noticed that getToplevelMessages[1] was not behaving as I expected it to.
> >>> Specifically, there were two places where I wanted different behavior:
> >>>
> >>> 1. The function takes an optional boolean argument, recent_first. When true,
> >>> the method sorts the threads by their modification dates, with the thread with
> >>> the most recent response listed first. Unexpectedly (at least to me), when
> >>> recent_first is false (the default), the threads are returned sorted by their
> >>> dates, i.e., by when the initial message was sent. Thus this is *not*
> >>> necessarily the reverse ordering from when specifying recent_first = True.
> >>>
> >>> 2. getToplevelMessages also takes optional start and end arguments for
> >>> specifying a date range. Again, this range uses the dates and not the
> >>> modification dates of the messages. This leads to confusion, particularly when
> >>> sorting on the modification, since you will likely not get the results you
> >>> expect (e.g., you'll likely get messages that you thought should have been
> >>> filtered out).
> >>>
> >>> I'd like to modify the function to both sort and filter by modification_date.
> >>> This seems to me to be more sensible behavior, particularly because it provides
> >>> the expected behavior when dealing with threads. I've grep'd through the code
> >>> and it doesn't look like it will break anything, but I wanted to shoot an
> >>> e-mail out to the list to make sure no one was relying on the current behavior
> >>> or had some reason to prefer it.
> >> i agree that we should consider some changes here, but i'm not completely 
> >> clear on what you're proposing.  for instance, for #1 above, i would not 
> >> expect recent_first being False to necessarily reverse the order in which the 
> >> threads are returned, so if that's what you're thinking it would do i'm not 
> >> sure i like that idea.
> >>
> > 
> > Yes, that is what I was thinking. I agree that setting recent_first to False
> > does not *necessarily* (in the logical sense) need to return the reversed list,
> > however, I still think it's a good idea. At a minimum, the interface needs to
> > be more clearly defined, as right now the only way to know how things are
> > ordered is to read the actual code. Is there a present use case for doing otherwise?
> > 
> >> maybe what we need is a better API altogether.  instead of a single boolean, 
> >> we might accept a 'sort_key' argument and an optional 'descending' or 
> >> 'reversed' boolean argument.  this would allow for more expressiveness and 
> >> more clarity.  would doing things this way meet your needs?  does it 
> >> complicate things enough to make you balk at wanting to do so right now?
> >>
> > 
> > I'm +0 on this, mostly because I think adding this on top of recent_first would
> > be clunky, and doing away with recent_first would change the established
> > interface. My needs could definitely be met by this API, however, assuming there were
> > both sort_on and filter_on (or something similar) keywords.
> 
> i'd prefer that we not change the recent_first behavior, but instead deprecate 
> the use of that argument, so that if it's passed in to the method a 
> deprecation warning is sent (using the python 'warnings' package) to the log, 
> like so:
> 
> _marker = object()
> def getToplevelMessages(self, start=None, end=None, recent_first=_marker,
>                          sort_on=None, filter_on=None, descending=False):
>      if recent_first is not _marker:
>          warnings.warn("Use of the 'recent_first' argument to the "
>                        "getToplevelMessages method is deprecated. "
>                        "Please use the 'sort_on', 'filter_on', and "
>                        "'descending' arguments to achieve the desired "
>                        "results", DeprecationWarning)
>          # following would be code that sets 'sort_on' and 'descending'
>          # so as to emulate the correct behavior depending on what
>          # 'recent_first' was set to
> 
> as long as the default behavior (i.e. the behavior when neither recent_first 
> nor one of the new keyword args) is the same, this seems to me a reasonable 
> transition path.

To recap, here's what I did and how things are working now:

1. Assuming none of the new arguments (sort_on, filter_on or reversed) are
specified, the behavior is identical to before, so no existing code should
break.

2. If recent_first is set explicitly, a DeprecationWarning is written to the
log.

3. You can now choose to filter and sort messages by modification_date (i.e.,
date of most recent reply) in addition to date (which was previously the only
option).

4. I used 'reversed' instead of 'descending' as the boolean argument, since
that name is used for the same functionality in the getMessageReferrers method

5. I updated the doc string and interface definition accordingly. Note I
changed the default here to be recent_first=False, as this was incorrect before
(the docs said True when in the actual implementation it was False).

6. I updated the calls to getToplevelMessages elsewhere in the code to use the
new arguments instead of recent_first

These changes were made on the ui-improvements branch, which rmarianski and I
just merged to trunk.

-Nick

> 
> -r
>