On Mon, Apr 06 2015, "W. Trevor King" <wking at tremily.us> wrote:
> On Fri, Apr 03 2015, Jani Nikula <jani at 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