Date: Sun, 29 Jan 2012 16:34:27 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Dmitry Kurochkin <dmitry.kurochkin@gmail.com>
Subject: Re: [PATCH 1/6] emacs: move tag format validation to `notmuch-tag'
	function
Message-ID: <20120129213427.GF17991@mit.edu>
References: <1327725684-5887-1-git-send-email-dmitry.kurochkin@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1327725684-5887-1-git-send-email-dmitry.kurochkin@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

One philosophical nit below, but not enough to hold this up.

Quoth Dmitry Kurochkin on Jan 28 at  8:41 am:
> Before the change, tag format validation was done in
> `notmuch-search-operate-all' function only.  The patch moves it down
> to `notmuch-tag', so that all users of that function get input
> validation.
> ---
>  emacs/notmuch.el |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/emacs/notmuch.el b/emacs/notmuch.el
> index 72f78ed..84d7d0a 100644
> --- a/emacs/notmuch.el
> +++ b/emacs/notmuch.el
> @@ -522,6 +522,12 @@ 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
> +  (when (null tags) (error "No tags given"))

Since this is a non-interactive function and hence is meant to be
invoked programmatically, I would expect it to accept zero tags.
Unlike the following check, this is a UI-level check and thus, I
believe, belongs in interactive functions (even if that requires a
little duplication).

> +  (mapc (lambda (tag)
> +	  (unless (string-match-p "^[-+][-+_.[:word:]]+$" tag)
> +	    (error "Tag must be of the form `+this_tag' or `-that_tag'")))
> +	tags)
>    (run-hooks 'notmuch-before-tag-hook)
>    (apply 'notmuch-call-notmuch-process
>  	 (append (list "tag") tags (list "--" query)))
> @@ -890,12 +896,6 @@ characters as well as `_.+-'.
>    (interactive (notmuch-select-tags-with-completion
>  		"Operations (+add -drop): notmuch tag "
>  		'("+" "-")))
> -  ;; Perform some validation
> -  (when (null actions) (error "No operations given"))
> -  (mapc (lambda (action)
> -	  (unless (string-match-p "^[-+][-+_.[:word:]]+$" action)
> -	    (error "Action must be of the form `+this_tag' or `-that_tag'")))
> -	actions)
>    (apply 'notmuch-tag notmuch-search-query-string actions))
>  
>  (defun notmuch-search-buffer-title (query)