On Sun, 08 Jan 2012 18:49:56 -0800, Jameson Graef Rollins <jrollins at finestructure.net> wrote: > 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.
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. > > > - 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. 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 ?d? deletes and ?C-u d? undeletes, for example.) In order to do that, the user?s code has to convert the prefix arg into a string. Making something ?lispy? isn?t 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?s an alternate proposal: provide two functions as the ?external? API, namely ?notmuch-show-{add,remove}-tag-thread? (by parallelism with ?notmuch-show-{add,remove}-tag?). These could be thin wrappers around ?notmuch-show-tag-thread-internal?, which would then not be intended to be called by user code. > > > 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. Using double parens is semantically correct, but makes the code less idiomatic and harder to read (IMO). To test my intuition, I looked at ?let? invocations in the Emacs source that have a non-initialized 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. -- Aaron Ecay