Re: [PATCH v2] emacs: wrap current search in parens when filtering

[Tomi Ollila <tomi.ollila@iki.fi>]

On Sun, Sep 06 2015, David Bremner <david@tethera.net> wrote:

> Some pretty fussy comments follow. Probably I could have fixed these in
> the time it took to write this message ;).
>
> Uli Scholler <uli@scholler.net> writes:
>> +  (let ((grouped-query (notmuch-maybe-group-query-string query))
>> +	(grouped-search-query (notmuch-maybe-group-query-string notmuch-search-query-string)))
>
> - I didn't find it very obvious which of these introduced variables was
>   which. I thought maybe "grouped-original-query" for the second
>   one. It's pretty subjective though, so your call.

Yes, naming is hard. I have an additional suggestion:

notmuch-maybe-group-query-string could be written as
notmuch-group-disjunctive-query-string

> - The lines get pretty long here. We try to keep code to 80 columns.

In this case the status quo did not change -- e.g. one line that was
replaced was even longer. Anyway, shorter lines would be welcome
improvements.

> - Your revised patch isn't in quite the right format for git am;
>   the actual commit message get's lost. The unintuitive trick is to add
>   commentary in the patch after the ---

Yes. A good custom is to try to git-am the file before sending if it has
been edited after git format-patch is used to create it -- or even if it
not edited, to catch any indesired "whitespace" etc. additions..


Tomi

>   
>> +    (notmuch-search (if (string= grouped-search-query "*")
>>  			grouped-query
>> -		      (concat notmuch-search-query-string " and " grouped-query)) notmuch-search-oldest-first)))
>> +		      (concat grouped-search-query " and " grouped-query)) notmuch-search-oldest-first)))
> _______________________________________________
> notmuch mailing list
> notmuch@notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch