From: Mark Walters <markwalters1009@gmail.com>
To: David Bremner <david@tethera.net>, notmuch@notmuchmail.org
Subject: Re: [PATCH v2 2/3] contrib: pick: remove hack
	notmuch-pick-show-match-message-with-wait
In-Reply-To: <87li5rtvbh.fsf@zancas.localnet>
References: <1372582512-10777-1-git-send-email-markwalters1009@gmail.com>
	<1372582512-10777-3-git-send-email-markwalters1009@gmail.com>
	<87li5rtvbh.fsf@zancas.localnet>
User-Agent: Notmuch/0.15.2+192~g8222af3 (http://notmuchmail.org) Emacs/23.4.1
	(x86_64-pc-linux-gnu)
Date: Sun, 30 Jun 2013 22:15:01 +0100
Message-ID: <87hagf5mi2.fsf@qmul.ac.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Precedence: list


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