Re: [PATCH v5 2/7] emacs: make the refresh functions more consistent

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

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