From: Aaron Ecay <aaronecay@gmail.com>
To: Jameson Graef Rollins <jrollins@finestructure.net>,
	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: <87boqdr3mz.fsf@servo.finestructure.net>
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> <87boqdr3mz.fsf@servo.finestructure.net>
User-Agent: Notmuch/0.10.1+56~gd709fd6 (http://notmuchmail.org)
	Emacs/24.0.92.3 (i386-apple-darwin10.8.0)
Date: Mon, 09 Jan 2012 00:02:20 -0500
Message-ID: <m2obudii3n.fsf@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: list

On Sun, 08 Jan 2012 18:49:56 -0800, Jameson Graef Rollins <jrollins@finestr=
ucture.net> wrote:
> Thanks so much for the review, Aaron.
>=20
> On Sun, 08 Jan 2012 20:08:59 -0500, Aaron Ecay <aaronecay@gmail.com> wrot=
e:
> > 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.
>=20
> That's a nice idea.  Probably better for a separate patch, though.

This patch introduces show-next as a new argument to the function.  So it
can and should make it &optional, if that is the appropriate semantics
for it to have.

>=20
> > - 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 val=
ue of =E2=80=98t=E2=80=99 would
> >   remove the tag; =E2=80=98nil=E2=80=99 would add it.  Moving this argu=
ment 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
>=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.

Using a string value makes it harder to interface with other code.  For
example, the prefix argument (C-u) is delivered to emacs commands as a
boolean value (nil if no arg, something truthy if the arg is given).  A
plausible custom end user function/keybinding would be one to add a tag
to the open messages, or remove that tag if the prefix arg is given to
the same command.  (So that =E2=80=98d=E2=80=99 deletes and =E2=80=98C-u d=
=E2=80=99 undeletes, for
example.)  In order to do that, the user=E2=80=99s code has to convert the
prefix arg into a string.  Making something =E2=80=9Clispy=E2=80=9D isn=E2=
=80=99t just about
code readability/saving keystrokes, but also refers to how well the
code interfaces with the conventions used by the rest of the emacs
environment.

That said, here=E2=80=99s an alternate proposal: provide two functions as t=
he
=E2=80=9Cexternal=E2=80=9D API, namely =E2=80=98notmuch-show-{add,remove}-t=
ag-thread=E2=80=99 (by
parallelism with =E2=80=98notmuch-show-{add,remove}-tag=E2=80=99).  These c=
ould be
thin wrappers around =E2=80=98notmuch-show-tag-thread-internal=E2=80=99, wh=
ich would
then not be intended to be called by user code.

>=20
> > No second set of parens is needed around tag-function.
>=20
> Yeah, I've seen this either way.  I guess it's just a stylistic
> choice.

Using double parens is semantically correct, but makes the code less
idiomatic and harder to read (IMO).  To test my intuition, I looked at
=E2=80=98let=E2=80=99 invocations in the Emacs source that have a non-initi=
alized
variable (because the multiple variable case is hard to grep for).
Double parens are used only 3% of the time (44 double vs 1468 single).
Make of this data what you will.

--=20
Aaron Ecay