Date: Mon, 6 Apr 2015 08:56:59 -0700
From: "W. Trevor King" <wking@tremily.us>
To: Tomi Ollila <tomi.ollila@iki.fi>
Subject: Re: [PATCH] nmbug-status: add support for specifying sort order for
 each view
Message-ID: <20150406155659.GS10467@odin.tremily.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
 protocol="application/pgp-signature"; boundary="gXx2FYK2AghGE4Yq"
Content-Disposition: inline
In-Reply-To: <1428047832-14922-1-git-send-email-jani@nikula.org>
OpenPGP: id=39A2F3FA2AB17E5D8764F388FC29BDCDF15F5BE8;
 url=http://tremily.us/pubkey.txt
User-Agent: Mutt/1.5.23 (2014-03-12)
Cc: notmuch@notmuchmail.org
Precedence: list


--gXx2FYK2AghGE4Yq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 =3D {
> +            '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 =3D view.get('sort', 'oldest-first')
  sort_attribute =3D sort_key.upper().replace('-', '_'))
  try:
      sort =3D 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

--=20
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

--gXx2FYK2AghGE4Yq
Content-Type: application/pgp-signature; name="signature.asc"
Content-Description: OpenPGP digital signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJVIqzIAAoJEG8/JgBt8ol8nuoP/1cPjZzGgmqy9ZtMts7dOCrC
C08gsUmiqgK6NWhAVnKzR4Fea38aUBSLnFcmQViU/XEfa3OwiBGCM2CxBbeUTE0V
3THjRdlVSiz36fIsrvaSP51wAF6DU6Qwaeb8/COPEaNm8GnsfwzQau1yjcOYJPij
HyI3BrOvxNd2y/6nJTjtbtoxt+FfrnhPTlA9gLlZEDDOCFTJx5p4ACDcxyoqlfXZ
B+4AbSyIiOXmjOhs3zULmZgp4RMYIMTZTcE8HnaXbkLZHsCTPZjyO7X1OhVRe59i
Hu/NyvU5eeFopfBw6EuW81wYp14piKJRiZdzDWWj0Vno+duDeMJBna25yaVwcVmA
1iRicnJ8BVEEAhhBokfCd74DKE8TTha6H0V5+majN4LwohuYlIoh4qxDEfAILeKC
uZv8j7gaqS2yeLNxARoxy0H5v5wVeh8TIvky1j/td1oG96XXC/pDX7N/tEBOBGDi
8PNTdZ5SHMDOnmaD3FTW1Ivq0Mkip3Rrtl1yC/hwfy1Ie04SN8C6YGiGeMFl6CRi
RlE2TMoz8+lqMqQ2BhtmrMp+FE59t4eT/Xl/lsPV/mlnjcoe67OMDOTOo9UdZIdE
qGVCg+Nxr45R7LA1eJm6P0d0Ai9iieur3dnQW9nLmbwEzvf/ERracZChaf1zgp3d
LArbR2GO8RY+xRTM7RFl
=1hYa
-----END PGP SIGNATURE-----

--gXx2FYK2AghGE4Yq--