Date: Tue, 12 Nov 2013 18:18:17 -0500
From: Austin Clements <amdragon@MIT.EDU>
To: Jameson Graef Rollins <jrollins@finestructure.net>
Subject: Re: [PATCH 8/8] emacs: Remove interactive behavior of `notmuch-tag'
Message-ID: <20131112231817.GE13399@mit.edu>
References: <1382471457-26056-1-git-send-email-amdragon@mit.edu>
	<1382471457-26056-9-git-send-email-amdragon@mit.edu>
	<8761s9gfpm.fsf@servo.finestructure.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8761s9gfpm.fsf@servo.finestructure.net>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Jameson Graef Rollins on Nov 03 at  4:42 pm:
> On Tue, Oct 22 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> > We no longer use this, since we've lifted all interactive behavior to
> > the appropriate interactive entry points.  Because of this,
> > `notmuch-tag' also no longer needs to return the tag changes list,
> > since the caller always passes it in.
> > ---
> >  emacs/notmuch-tag.el | 19 ++++---------------
> >  1 file changed, 4 insertions(+), 15 deletions(-)
> >
> > diff --git a/emacs/notmuch-tag.el b/emacs/notmuch-tag.el
> > index f9c1740..feee17c 100644
> > --- a/emacs/notmuch-tag.el
> > +++ b/emacs/notmuch-tag.el
> > @@ -249,27 +249,18 @@ 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 &optional tag-changes)
> > +(defun notmuch-tag (query tag-changes)
> >    "Add/remove tags in TAG-CHANGES to messages matching 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.
> > +TAG-CHANGES is a list of strings of the form \"+tag\" or
> > +\"-tag\" to add or remove tags, respectively.
> >  
> >  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
> > -			     (notmuch-tag-completions query) nil tag-changes))
> > -	(setq tag-changes (list tag-changes))))
> 
> Hey folks.  After a recent upgrade to 0.16+120~gfd733a4+1 I found that
> my custom tagging functions weren't working, which I traced back to the
> removal of this section.
> 
> Previously notmuch-tag accepted tag changes in two forms: as a list of
> tag changes:
> 
> (list "+foo" "-bar")
> 
> or as a space-separated string:
> 
> "+foo -bar"
> 
> This removed section is what would turn the space-separated string into
> a list.
> 
> I have custom tagging functions that were written like this:
> 
> (notmuch-search-tag "+spam")
> 
> notmuch-search-tag passes the TAG-CHANGES to notmuch-tag, which was
> changing it to a list in this removed section.  Since its removal, all
> of my custom functions started throwing the following error:
> 
> Wrong type argument: stringp, 43
> 
> I didn't pay super close attention to the rest of the patch series, but
> From a backwards compatibility point of view, do we really need to
> remove this section?  Is there a problem with specifying tag changes as
> a string?  I think it was actually me that last modified notmuch-tag,
> and I vaguely remember spending time thinking about how to preserve that
> particular way to specify the changes, so that things would be simpler
> for simple tag operations.

The difficulty is that notmuch-tag used to play a dual role of both
parsing the space-separated string into a list of tag changes and
actually performing the tagging operation and, as a result, other
functions that were rooted in notmuch-tag also accepted both (though
functions that weren't, like notmuch-update-tags, required the list
form).  This dual role was strange from an API perspective, but also,
because of other restructuring, notmuch-tag is no longer at the core
of many operations, so it's not in a good position to be a parser.

Hence, while we could make `notmuch-tag' itself support the string
form, if we wanted this to work from other tagging entry-points, we'd
need to support both forms in all of the tagging functions.  This
would be doable, but it would be messy, and without the single parsing
point it would be difficult to maintain.

> In any event, some amount of backwards compatibility has been removed,
> so if we do want to keep it removed, we should add a good NEWS item to
> explain that peoples custom bindings might break.

I'd rather keep the backwards compatibility removed, but you're
definitely right that we need a good NEWS item for it.  I've posted a
NEWS item in id:1384296252-17884-1-git-send-email-amdragon@mit.edu,
but let me know if you think it can be clearer or more helpful.

> jamie.
> 
> >    (mapc (lambda (tag-change)
> >  	  (unless (string-match-p "^[-+]\\S-+$" tag-change)
> >  	    (error "Tag must be of the form `+this_tag' or `-that_tag'")))
> > @@ -278,9 +269,7 @@ 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))
> > -  ;; in all cases we return tag-changes as a list
> > -  tag-changes)
> > +    (run-hooks 'notmuch-after-tag-hook)))
> >  
> >  (defun notmuch-tag-change-list (tags &optional reverse)
> >    "Convert TAGS into a list of tag changes.
> >
> > _______________________________________________
> > notmuch mailing list
> > notmuch@notmuchmail.org
> > http://notmuchmail.org/mailman/listinfo/notmuch