On Mon, Oct 10 2016, Mark Walters <markwalters1009@gmail.com> wrote: > On Sun, 09 Oct 2016, Tomi Ollila <tomi.ollila@iki.fi> wrote: >> On Sun, Oct 09 2016, Mark Walters <markwalters1009@gmail.com> wrote: >> >>> diff --git a/emacs/notmuch-lib.el b/emacs/notmuch-lib.el >>> index b2cdace..8b55ca7 100644 >>> --- a/emacs/notmuch-lib.el >>> +++ b/emacs/notmuch-lib.el >>> @@ -416,11 +416,10 @@ of its command symbol." >>> (defun notmuch-refresh-this-buffer () >>> "Refresh the current buffer." >>> (interactive) >>> - (when notmuch-buffer-refresh-function >>> - (if (commandp notmuch-buffer-refresh-function) >>> - ;; Pass prefix argument, etc. >>> - (call-interactively notmuch-buffer-refresh-function) >>> - (funcall notmuch-buffer-refresh-function)))) >>> + (when (and notmuch-buffer-refresh-function >>> + (commandp notmuch-buffer-refresh-function)) >>> + ;; Pass prefix argument, etc. >>> + (call-interactively notmuch-buffer-refresh-function))) > > Hi > > Thanks for the review. > >> If there is going to be more rounds, IMO this (currently wrong, missing >> second argument t) commandp check should be dropped. It is better to >> signal programmer error than silently ignore non-nil variable that is >> not referring to interactive function (the code is also simpler this way). > > Yes I agree. Do you have any preference between just dropping the test, > and putting our own error instead? I'd just drop the test. e.g. (if/when notmuch-buffer-refresh-function (call-interactively notmuch-buffer-refresh-function)) Tomi > >> and a quesstion: the last patch in this series defines this >> refresh all buffers function -- why is it not interactive ? > > Because I forgot to amend the last commit :-) The interactive is in my > tree, and was in what I tested -- I agree it should be. > > Best wishes > > Mark > > >> Otherwise series looks good to me. > > >> >> Tomi