From: Tomi Ollila <tomi.ollila@iki.fi>
To: "W. Trevor King" <wking@tremily.us>
Subject: Re: [PATCH] nmbug-status: add support for specifying sort order for
 each view
In-Reply-To: <20150406155659.GS10467@odin.tremily.us>
References: <20150406155659.GS10467@odin.tremily.us>
User-Agent: Notmuch/0.19+107~gab55bdb (http://notmuchmail.org) Emacs/24.3.1
 (x86_64-unknown-linux-gnu)
Date: Mon, 06 Apr 2015 19:48:09 +0300
Message-ID: <m2r3rx9p9y.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain
Cc: notmuch@notmuchmail.org
Precedence: list

On Mon, Apr 06 2015, "W. Trevor King" <wking@tremily.us> wrote:

> On Fri, Apr 03 2015, Jani Nikula <jani@nikula.org> wrote:
>> Let each view have "sort" key with possible values "oldest-first",
>> "newest-first", and "unsorted", and sort the results
>> accordingly. Oldest first remains the default.
>
> I like it, but have a few suggestions to tweak the implementation.
>
>>      def _write_view(self, database, view, stream):
>> +        sort = {
>> +            'oldest-first': notmuch.Query.SORT.OLDEST_FIRST,
>> +            'newest-first': notmuch.Query.SORT.NEWEST_FIRST,
>> +            'unsorted': notmuch.Query.SORT.UNSORTED
>> +        }
>
> I'd rather have this mapping defined in a global variable
> (_SORT_TERMS?) or a class-wide attribute (Page.sort_terms?).
> Alternatively, you could do something dynamic like:
>
> 	sort_key = view.get('sort', 'oldest-first')
>   sort_attribute = sort_key.upper().replace('-', '_'))
>   try:
>       sort = getattr(notmuch.Query.SORT, sort_attribute)
>   except AttributeError:
>       raise ConfigError('Invalid setting for {}: {!r}'.format(
>           view['title'], sort_key))
>
> which would automatically keep the implementation in sync with the
> available values in notmuch.Query.SORT.

I like how this suggestion looks like.

Tomi

>
>> -        q.set_sort(notmuch.Query.SORT.OLDEST_FIRST)
>> +        if 'sort' in view and view['sort'] in sort:
>> +            q.set_sort(sort[view['sort']])
>> +        else:
>> +            q.set_sort(notmuch.Query.SORT.OLDEST_FIRST)
>
> Instead of silently falling back to oldest-first if the requested
> sort-key isn't available, I think we should be raising ConfigError so
> the user knows they need to update their config.
>
> Cheers,
> Trevor
>
> -- 
> This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
> For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy