Re: [PATCH] nmbug-status: add support for specifying sort order for each view

["W. Trevor King" <wking@tremily.us>]

[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]

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.

> -        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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]