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
>
--
Archive:
http://www.openplans.org/projects/listen/lists/listen-dev/archive/2009/01/1233262299283
To unsubscribe send an email with subject "unsubscribe" to
[email protected]. Please contact
[email protected] for questions.
