From: Mark Walters <markwalters1009@gmail.com>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [PATCH 2/2] contrib: pick: thread tagging (including archiving)
	implemented
In-Reply-To: <87zjv91uki.fsf@zancas.localnet>
References: <1354970914-18342-1-git-send-email-markwalters1009@gmail.com>
	<1354970914-18342-3-git-send-email-markwalters1009@gmail.com>
	<87zjv91uki.fsf@zancas.localnet>
User-Agent: Notmuch/0.14+255~gff3cc55 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sat, 01 Jun 2013 22:45:27 +0100
Message-ID: <87r4gly07c.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


Thanks for the review. A couple of comments below:

On Sat, 01 Jun 2013, David Bremner <david@tethera.net> wrote:
> Mark Walters <markwalters1009@gmail.com> writes:
>
>> Previously pick had no actions based on the entire thread: this adds
>> some. Note in this version '*' is bound to `tag thread' which is not
>> consistent with search or show. However it still might be the most
>> natural thing (as it is similar to running * in the show pane).
>
> Hmm. not sure about this. But since nobody complained in six months,
> let's go with it unless they wake up.

Ok

>> +  (setq tag-changes (funcall 'notmuch-tag
>> (notmuch-pick-get-messages-ids-thread-search) tag-changes))
>
> Why "(funcall 'notmuch-tag ...) instead of a "(notmuch-tag ...)" here?
> Personally I'd use let instead of setq here, but ymmv.

I agree with both of these but this is almost identical code to that in
notmuch-show-tag-all. 

>> +Archive each message currently shown by applying the tag changes
>> +in `notmuch-archive-tags' to each (remove the \"inbox\" tag by
>> +default). If a prefix argument is given, the messages will be
>> +\"unarchived\", i.e. the tag changes in `notmuch-archive-tags'
>> +will be reversed.
>
> It seems to me the default value for notmuch-archive-tags should not be
> hard-coded into this docstring, especially since notmuch-archive-tags
> turns into a link in the help text.

I am happy either way on this one but again this is the same as
notmuch-show-archive-thread.

I think it's probably best to keep show and pick the same where they are
similar but I can change the above if you disagree.

Best wishes

Mark