From: Mark Walters <markwalters1009@gmail.com>
To: Austin Clements <amdragon@MIT.EDU>
Subject: Re: [PATCH 0/8] Improve tag change completion
In-Reply-To: <20131023001952.GD20337@mit.edu>
References: <1382471457-26056-1-git-send-email-amdragon@mit.edu>
	<87mwm1x9pi.fsf@qmul.ac.uk> <20131023001952.GD20337@mit.edu>
User-Agent: Notmuch/0.16 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Wed, 23 Oct 2013 10:56:59 +0100
Message-ID: <87fvrsxqc4.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: notmuch@notmuchmail.org
Precedence: list


Hi

On Wed, 23 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
> Quoth Mark Walters on Oct 22 at 10:43 pm:
>> This looks good to me +1. It makes the code clearer and nicer to read as
>> well as giving a better user experience, and it is makes fixing the long
>> standing tagging races simpler.
>> 
>> I have a couple of docstring comments:
>> 
>> In patch 2 perhaps notmuch-tag-completions could have a docstring.
>
> Added.  I noticed that I had failed to update the call from
> `notmuch-select-tag-with-completion', so I fixed that, too.  I don't
> understand why we take lists of search terms in random places and
> never use more than one element, but I suppose this series doesn't
> make that any worse.

As far as I can see, at the end of the series, notmuch-tag-completions
is only called with no argument: i.e., it's always just finding the list
of all tags. This is because notmuch-select-tag-with-completion is only
called once from "notmuch-search-filter-by-tag" with no search-terms
argument. So it might be nice to just remove the search-terms
completely. (The only downside is we might break user lisp.)

Best wishes

Mark


>> In Patch 4 I think the docstring for notmuch-search-tag is outdated: it
>>  is "Change tags for the currently selected thread or region." but 
>>  beg and end can now be specified by the caller.
>
> I've left the first sentence as it is, since it's good interactive
> documentation and a typical way to describe functions even if they
> take a region as arguments (see, for example, `kill-region').  But
> I've elaborated the rest of the docstring to be clearer about this.
>
>> and one actual comment:
>> 
>> in patch 3 (for show) delete-dups is called before the list is passed to
>> notmuch-read-tag-changes whereas it is not for search or pick.
>> Obviously this is not actually a problem but it might be worth being
>> consistent.
>
> Ah, whoops.  I'd done this before I decided to handle duplicates in
> `notmuch-read-tag-changes'.  Since it's redundant, I've removed it.
>
>> But that was all I found. All tests pass and everything I try behaves
>> exactly as expected.
>> 
>> Best wishes
>> 
>> Mark
>> 
>> 
>> On Tue, 22 Oct 2013, Austin Clements <amdragon@MIT.EDU> wrote:
>> > This series improves tag change completion in various ways for
>> > commands like +, -, and *.
>> >
>> > From a user perspective, this provides command-specific prompts like
>> > "Tag message" and "Tag all" instead of the generic "Tag" prompt, and
>> > bases tag removal completions on the tags that are in the buffer,
>> > rather than the current tags in the database, providing a more
>> > predicable experience.
>> >
>> > From an implementation perspective, this new tag removal completion
>> > behavior improves efficiency and eliminates a road block to fixing the
>> > tagging race bug (which otherwise results in massive queries just to
>> > compute removal completions).  The new code is also more "Elispy" and
>> > predictable because all tag change prompting now occurs at the
>> > interactive entry points, rather than buried under several layers of
>> > non-interactive calls.
>> >
>> > This is a spiritual successor to
>> > id:1354263691-19715-1-git-send-email-markwalters1009@gmail.com, though
>> > it takes a very different approach.  This is also a prerequisite to
>> > the tag race fix in
>> > id:1381185201-25197-1-git-send-email-amdragon@mit.edu and I plan to
>> > send an updated version of that series when this one is accepted.
>> >
>> > Patches 1, 5, and 6 could be pushed on their own.  They fix bugs or
>> > sort of bugs that get in the way of the rest of the series.