Re: [PATCH v2] emacs: wrap current search in parens when filtering
On Sun, Sep 06 2015, David Bremnerwrote: > Some pretty fussy comments follow. Probably I could have fixed these in > the time it took to write this message ;). > > Uli Scholler 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 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH v2] emacs: wrap current search in parens when filtering
Some pretty fussy comments follow. Probably I could have fixed these in the time it took to write this message ;). Uli Schollerwrites: > + (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. - The lines get pretty long here. We try to keep code to 80 columns. - 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 --- > +(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
[PATCH v2] emacs: wrap current search in parens when filtering
Tomi Ollilawrites: > But should this do the same "notmuch-search-disjunctive-regexp" check > which is done when building up `grouped-query' ? Here is an improved version of my patch: --- When filtering the current search further with notmuch-search-filter, wrap the current search in parens if necessary. This fixes unexpected behavior when the current search is complex (like "(tag:this and date:one_week_ago..) or tag:that"). --- emacs/notmuch.el | 18 +- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/emacs/notmuch.el b/emacs/notmuch.el index 5284e77..e5e677f 100644 --- a/emacs/notmuch.el +++ b/emacs/notmuch.el @@ -978,18 +978,26 @@ default sort order is defined by `notmuch-search-oldest-first'." (set 'notmuch-search-oldest-first (not notmuch-search-oldest-first)) (notmuch-search-refresh-view)) +(defun notmuch-maybe-group-query-string (query-string) + "Group query if it contains a complex expression. + +Enclose QUERY-STRING in parentheses if it matches +`notmuch-search-disjunctive-regexp'." + (if (string-match-p notmuch-search-disjunctive-regexp query-string) + (concat "( " query-string " )") +query-string)) + (defun notmuch-search-filter (query) "Filter the current search results based on an additional query string. Runs a new search matching only messages that match both the current search results AND the additional query string provided." (interactive (list (notmuch-read-query "Filter search: "))) - (let ((grouped-query (if (string-match-p notmuch-search-disjunctive-regexp query) - (concat "( " query " )") -query))) -(notmuch-search (if (string= notmuch-search-query-string "*") + (let ((grouped-query (notmuch-maybe-group-query-string query)) + (grouped-search-query (notmuch-maybe-group-query-string notmuch-search-query-string))) +(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))) (defun notmuch-search-filter-by-tag (tag) "Filter the current search results based on a single tag. -- 2.1.4 ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch