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>