[PATCH 2/4] emacs: repurpose notmuch-show-archive-thread-internal function for general thread tagging

[Jameson Graef Rollins]

Thanks so much for the review, Aaron.

On Sun, 08 Jan 2012 20:08:59 -0500, Aaron Ecay <aaronecay at 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 “remove”; a value of ‘t’ would
>   remove the tag; ‘nil’ would add it.  Moving this argument after ‘tag’
>   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.) 

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 835 bytes
Desc: not available
URL: <http://notmuchmail.org/pipermail/notmuch/attachments/20120108/ebb248b7/attachment.pgp>