From: Jameson Graef Rollins <jrollins@finestructure.net>
To: Aaron Ecay <aaronecay@gmail.com>, Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: [PATCH 2/4] emacs: repurpose notmuch-show-archive-thread-internal
	function for general thread tagging
In-Reply-To: <m239bpk7h0.fsf@gmail.com>
References: <1325975294-646-1-git-send-email-jrollins@finestructure.net>
	<1325975294-646-2-git-send-email-jrollins@finestructure.net>
	<1325975294-646-3-git-send-email-jrollins@finestructure.net>
	<m239bpk7h0.fsf@gmail.com>
User-Agent: Notmuch/0.10.2+168~g34b8bac (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Sun, 08 Jan 2012 18:49:56 -0800
Message-ID: <87boqdr3mz.fsf@servo.finestructure.net>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha256; protocol="application/pgp-signature"
Precedence: list

--=-=-=
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Thanks so much for the review, Aaron.

On Sun, 08 Jan 2012 20:08:59 -0500, Aaron Ecay <aaronecay@gmail.com> wrote:
> A couple of comments on the arguments:
> - It would be good to make show-next &optional.  This will enable code
>   to call the fn with only two arguments, and not showing next will be
>   the default behavior.

That's a nice idea.  Probably better for a separate patch, though.

> - A more lispy way of specifying the sign would be to use a
>   boolean.  Perhaps you could call this =E2=80=9Cremove=E2=80=9D; a value=
 of =E2=80=98t=E2=80=99 would
>   remove the tag; =E2=80=98nil=E2=80=99 would add it.  Moving this argume=
nt after =E2=80=98tag=E2=80=99
>   and also making it &optional woudl allow this fn to be called with one
>   arg to add a tag.  (Maybe this is too minimalist and API, however.)=20

That might be more lispy, but it seems a lot less clear to me.  It might
save a few keystrokes when coding, but it would definitely make the code
a lot harder to read ("remove" to add a tag?).  I think I would prefer
people to give the sign explicitly.

> No second set of parens is needed around tag-function.

Yeah, I've seen this either way.  I guess it's just a stylistic choice.

I think it might make sense, but again I think that's out of the scope
of this patch series.  The point was to make a minimal set of
modifications here.  If we want to separate out the functionality, we
should do that in a separate patch.

Thanks again for the review.

jamie.

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iQIcBAEBCAAGBQJPClXVAAoJEO00zqvie6q88GcP/jPFYZv1beZqV9ohNEpF8vfo
Jz+9rrKhW+85Sb2p6yjRq7F9zt4L6HBCLyhL6iIUOMgyD3RI/cSSGlOVqzc/sxPk
8r5lYYIVa4J+61zXCh9anTh8VqKzCmVoVSGTlzCjzj4dfKAIyfI+HR7z86H3Egf3
xDD5iMyt2SfiWOYmL3gCu2uYNKGvZ21okiRLbbbpfg5jZS9S/mZKlR9FvILxN1Le
ftpEakv7qUkqfPL5yZOyZeoQ2/PyRf6hGeWswnqF/jJsliKhh0YmwkYkDcXk31XV
2pSCAZcHJZCiFdZlA0OX1OJUWYLDlyeiKOOxZQcJMT1iUOTRN+YhGhRAy3NlmKvn
9Oe3iuLhP0f7WG7/bXeuCAuO3LbQkaCZZnaDBHw3lz0coJPyM+2SYLnd2XJj9Q6T
KRTP0ZwTNN/L0DBDmfS0i45oxFj4JXna5GVHfKnwTTf5EeXegs38VwAFgVY6I3ba
Qki5KgTn1mNkmQBUIWYOwbb0HeIvaUhMInJKzoPWF3w42ZBUwa5Ak3LZ66lca3OW
YuHZN3XRiiwOPNr4RHMT/ryrPvkZMq0/k1ejJVQ7A+EP5dVEfwBGGuwUBkHOX/Pj
Bt1bPyNClwNblND3XbxXffP8W5iYpO4K8gQUvJjvMvUcsQr/yVJ1RFCyJjhxrbHr
Jd+TLFsE/6X4XMX3kQ/H
=46f2
-----END PGP SIGNATURE-----
--=-=-=--