On Mon, 11 Jul 2011 10:42:04 +0200, Felix Geller <fgeller@gmail.com> wrote:
> I added a variable to toggle message indentation in Emacs.
Hi, Felix. Thanks for submitting this patch. I think it's a good idea.
I have a couple of comments below, a couple of which echo what Dmitry
has already pointed out.
> diff --git a/emacs/notmuch-show.el b/emacs/notmuch-show.el
This patch doesn't include a commit log, which is something we generally
require. The preferred way to send patches is with git format-patch or
send-email, both of which format patches in such a way that they can be
immediately applied to a git repo, including with the commit log.
> +(defcustom notmuch-show-indent-messages-in-thread nil
> + "Should the messages in a thread be indented according to their respective depth in the thread?"
> + :group 'notmuch
> + :type 'boolean)
I agres with Dmitry that this should default to 't', to be consistent
with the current default behavior.
> - (insert (notmuch-show-spaces-n depth)
> + (insert (if notmuch-show-indent-messages-in-thread
> + (notmuch-show-spaces-n depth)
> + "")
I also agree with Dmitry's suggestion here to use the following slightly
simpler construct:
(if notmuch-show-indent-messages-in-thread
(insert (notmuch-show-spaces-n depth)))
Finally, as Dmitry also points out, you'll almost certainly need to
construct a test for this feature, since it constitutes a pretty big
formatting change. It should probably test for both cases of the
customization variable. Check out the tests in tests/emacs for
guidance.
hth.
jamie.