From: Mark Walters <markwalters1009@gmail.com>
To: Jameson Graef Rollins <jrollins@finestructure.net>,
	Notmuch Mail <notmuch@notmuchmail.org>
Subject: Re: [PATCH 2/5] emacs: allow notmuch-tag to accept string inputs and
	prompt for tags
In-Reply-To: <1334429574-12918-3-git-send-email-jrollins@finestructure.net>
References: <1333845338-22960-1-git-send-email-jrollins@finestructure.net>
	<1334429574-12918-1-git-send-email-jrollins@finestructure.net>
	<1334429574-12918-2-git-send-email-jrollins@finestructure.net>
	<1334429574-12918-3-git-send-email-jrollins@finestructure.net>
User-Agent: Notmuch/0.12+110~gbc97b4a (http://notmuchmail.org) Emacs/23.3.1
	(x86_64-pc-linux-gnu)
Date: Sat, 14 Apr 2012 21:35:09 +0100
Message-ID: <878vhyt5ky.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list

On Sat, 14 Apr 2012, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> notmuch-tag is extended to accept various formats of the tag changes.
> In particular, user prompting for tag changes is now incorporated
> here, so it is common for modes.
>
> The tag binary and the notmuch-{before,after}-tag-hooks are only
> called if tag changes is non-nil.
>
> The actual tag-changes applied are returned by the function.
> ---
>  emacs/notmuch-tag.el |   20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> index c25cff8..dd7f9d7 100644
> --- a/emacs/notmuch-tag.el
> +++ b/emacs/notmuch-tag.el
> @@ -108,18 +108,26 @@ from TAGS if present."
>  	   (error "Changed tag must be of the form `+this_tag' or `-that_tag'")))))
>      (sort result-tags 'string<)))
>  
> -(defun notmuch-tag (query &rest tag-changes)
> +(defun notmuch-tag (query &optional tag-changes)
>    "Add/remove tags in TAG-CHANGES to messages matching QUERY.
>  
> -TAG-CHANGES should be a list of strings of the form \"+tag\" or
> -\"-tag\" and QUERY should be a string containing the
> -search-query.
> +QUERY should be a string containing the search-terms.
> +TAG-CHANGES can take multiple forms.  If TAG-CHANGES is a list of
> +strings of the form \"+tag\" or \"-tag\" then those are the tag
> +changes applied.  If TAG-CHANGES is a string then it is
> +interpreted as a single tag change.  If TAG-CHANGES is the string
> +\"-\" or \"+\", or null, then the user is prompted to enter the
> +tag changes.
>  
>  Note: Other code should always use this function alter tags of
>  messages instead of running (notmuch-call-notmuch-process \"tag\" ..)
>  directly, so that hooks specified in notmuch-before-tag-hook and
>  notmuch-after-tag-hook will be run."
>    ;; Perform some validation
> +  (if (string-or-null-p tag-changes)
> +      (if (or (string= tag-changes "-") (string= tag-changes "+") (null tag-changes))
> +	  (setq tag-changes (notmuch-read-tag-changes tag-changes query))
> +	(setq tag-changes (list tag-changes))))
>    (mapc (lambda (tag-change)
>  	  (unless (string-match-p "^[-+]\\S-+$" tag-change)
>  	    (error "Tag must be of the form `+this_tag' or `-that_tag'")))
> @@ -128,7 +136,9 @@ notmuch-after-tag-hook will be run."
>      (run-hooks 'notmuch-before-tag-hook)
>      (apply 'notmuch-call-notmuch-process "tag"
>  	   (append tag-changes (list "--" query)))
> -    (run-hooks 'notmuch-after-tag-hook)))
> +    (run-hooks 'notmuch-after-tag-hook))

Hi

The series looks good to me with one minor point:

> +  ;; return the list of actual changed tags
> +  tag-changes)

I found the comment confusing: I read it as the function looked at the
tags the message had before and after and returned the
difference. Perhaps something like "in all cases we return tag-changes
as a list" (and a similar comment for the commit message).

Best wishes

Mark