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 “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.