Hi

Many thanks for the review!

On Sun, 30 Jun 2013, David Bremner <david@tethera.net> wrote:
> Mark Walters <markwalters1009@gmail.com> writes:
>
>> +(defvar notmuch-pick-open-target nil)
>> +(make-variable-buffer-local 'notmuch-pick-open-target)
>
> What do people think about adding a code style suggestion/requirement
> for elisp that all variables have docstrings, even if intended for
> internal use?  It's true the existing code doesn't really meet this
> standard.

I think this would be a good idea (but see below): I assume this is
anything defined with a defvar?

>>  (defvar notmuch-pick-buffer-name nil)
>>  (make-variable-buffer-local 'notmuch-pick-buffer-name)
>>  ;; This variable is the window used for the message pane. It is set
>> @@ -349,8 +351,8 @@ Does NOT change the database."
>>    (notmuch-pick (notmuch-search-find-thread-id)
>>                  notmuch-search-query-string
>>  		nil
>> -                (notmuch-prettify-subject (notmuch-search-find-subject)))
>> -  (notmuch-pick-show-match-message-with-wait))
>> +                (notmuch-prettify-subject (notmuch-search-find-subject))
>> +		t))
>
> I think my previous complaint can be reformulated as (essentially) both
> notmuch-pick and notmuch-pick-open-target could use (better) docstrings.

I will send a patch to add a docstring to the main notmuch-pick function
as a reply to this message. The exact style was unclear (we seem to do
different things in different places).

I am not sure what the best way to document the variable is: there are
several variables that are essentially buffer local versions of the
arguments passed to notmuch-pick. Should these duplicate the
documentation? Exactly the same situation occurs with notmuch-show in
notmuch-show.el and notmuch-search in notmuch.el and in both those cases
the functions are well documented but the variables are not documented.

Any suggestions?

Best wishes

Mark



> As you say, the hack removed is quite horrible, so I'd be willing to
> merge the patches anyway. OTOH, more documentation might make it so that
> more than one person can understand the notmuch-pick code.
>
> d