Re: [PATCH v2] Add notmuch-show--build-queries.

[Matt Armstrong <marmstrong@google.com>]

Mark, thanks for the review.  I agree with your comments.  I'll send
another patch.

As you might guess, I'm new to elisp.  Use of push makes some of this
fall out nicely.

For what it is worth, I did experiment with using loop to find the first
functional "forest".  There are several other calls to loop in this
file.  It was something like:

(let* ((cli-args (....))
       (forest (loop for query in (notmuch-show--build-queries)
                     for forest = (notmuch-query-get-threads
                                   (append cli-args
                                           (list "'") query (list "'")))
                     until forest
                     finally return forest))
      ...)
  ...)


I won't go that route in my next patch (I'm not convinced it is better).
But I also am not particularly invested in any one way to do this.



Mark Walters <markwalters1009@gmail.com> writes:

> On Tue, 11 Oct 2016, Matt Armstrong <marmstrong@google.com> wrote:
>> notmuch-show--build-buffer now queries a list of queries built by the
>> former.  This simplifies the logic.  It also provides an easy place to
>> experiment with alternate sets of queries for given notmuch-show-*
>> variables (e.g. users can use advice-add to do so in a surgical way).
>
> Hi
>
> I think I like the overall logic, but I do have some comments below.
>
>> ---
>>  emacs/notmuch-show.el | 37 +++++++++++++++++++++++--------------
>>  1 file changed, 23 insertions(+), 14 deletions(-)
>>
>> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
>> index f2487ab..0727319 100644
>> --- a/emacs/notmuch-show.el
>> +++ b/emacs/notmuch-show.el
>> @@ -1261,6 +1261,20 @@ matched."
>>  	(message "No messages matched the query!")
>>  	nil))))
>>  
>> +(defun notmuch-show--build-queries ()
>> +  "Return a list of queries to try for this search.
>> +
>> +If `notmuch-show-query-context` is not nil, the first query is
>> +the conjunction of it and `notmuch-show-thread-id`.  The next
>> +query is `notmuch-show-thread-id` alone, and serves as a fallback
>> +if the prior matches no messages."
>> +  (let* ((thread notmuch-show-thread-id)
>> +	 (context notmuch-show-query-context)
>> +	 (queries `((,thread))))
>> +    (if context
>> +	(setq queries (cons `(,thread "and (" ,context ")") queries)))
>> +    queries))
>
> I think it would be better to keep closer to the existing style -- using
> list and append. At least do that as a first patch and then do a change
> as above with ` and , so we can review them separately. Note I find the
> list append style much easier to read, and I am guessing someone else
> did (perhaps cworth?) as they wrote it that way.
>
> I think I would go for something like
>
> (let* ((...
>     queries))
>   (push (list thread) queries)
>   (if context (push (list thread "and (" context ")") queries))
>   queries)



>>  (defun notmuch-show--build-buffer (&optional state)
>>    "Display messages matching the current buffer context.
>>  
>> @@ -1268,25 +1282,20 @@ Apply the previously saved STATE if supplied, otherwise show the
>>  first relevant message.
>>  
>>  If no messages match the query return NIL."
>> -  (let* ((basic-args (list notmuch-show-thread-id))
>> -	 (args (if notmuch-show-query-context
>> -		   (append (list "\'") basic-args
>> -			   (list "and (" notmuch-show-query-context ")\'"))
>> -		 (append (list "\'") basic-args (list "\'"))))
>> -	 (cli-args (cons "--exclude=false"
>> +  (let* ((cli-args (cons "--exclude=false"
>>  			 (when notmuch-show-elide-non-matching-messages
>>  			   (list "--entire-thread=false"))))
>> -
>> -	 (forest (or (notmuch-query-get-threads (append cli-args args))
>> -		     ;; If a query context reduced the number of
>> -		     ;; results to zero, try again without it.
>> -		     (and notmuch-show-query-context
>> -			  (notmuch-query-get-threads (append cli-args basic-args)))))
>> -
>> +	 (queries (notmuch-show--build-queries))
>> +	 (forest nil)
>>  	 ;; Must be reset every time we are going to start inserting
>>  	 ;; messages into the buffer.
>>  	 (notmuch-show-previous-subject ""))
>> -
>> +    ;; Use results from the first query that returns some.
>> +    (while (and (consp queries)
>> +		(not (setq forest
>> +			   (notmuch-query-get-threads
>> +			    `(,@cli-args "'" ,@(car queries) "'")))))
>> +      (setq queries (cdr queries)))
>
> Similarly here I would avoid the ` , @ . I think I would also suggest
> making pushing the setq outside the while condition. Something like
>
> (while (and (consp queries)
>             (not forest))
>    (setq forest (notmuch-query-get-threads
>                   (append cli-args (list "'") (car queries) (list "'"))))