Re: [PATCH -v2] notmuch.el: Support for customizing search result display

[Carl Worth <cworth@cworth.org>]

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

On Wed,  2 Dec 2009 18:19:38 +0530, "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> wrote:
> From: Aneesh Kumar K.V <aneesh.kumar@gmail.com>
> 
> This patch helps in customizing search result display
> similar to mutt's index_format. The customization is done
> by defining an alist as below
> 
> (setq notmuch-search-result-format '(("date" . "%s ")
> 				     ("authors" . "%-40s ")
> 				     ("subject" . "%s ")
> 				     ("tags" . "(%s)")))
> 
> The supported keywords are date, count, authors, subject and tags.
> 
> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@gmail.com>

Hi Aneesh,

I'm sorry this patch has lingered so long without comment. There's
really only one problem I see with it:

> +(defcustom notmuch-search-result-format nil
> +  "Search result formating. Supported fields are
> +	date, count, authors, subject, tags
> +ex: (setq notmuch-search-result-format \(\(\"authors\" . \"%-40s\"\)
> +					\(\"subject\" . \"%s\"\)\)\)"
> +:type '(alist :key-type (string) :value-type (string))
> +:group 'notmuch)

...

> -			(insert (format "%s %-7s %-40s %s (%s)\n" date count authors subject tags))
> +			(if (not notmuch-search-result-format)
> +			    (progn (insert (format "%s %-7s %-40s %s" date count authors subject))
> +				   ;; insert the fontified tag
> +				   (insert-tags (format "%s" tags))
> +				   (insert "\n"))
> +			  (notmuch-search-show-result date count authors subject tags))

I don't like that the new format variable is nil by default and then
there's an open-coded implementation of the default formatting. This has
a couple of problems:

1. The new code is not being exercised by default, so it would be easy
   to break it without realizing.

2. The system is not very self-documenting.

   If a new user wants to tweak the default format, that will be a lot
   easier if they find a customizable variable that describes the
   current format. *That* will be a lot easier to modify rather than
   trying to learn what should be used instead of "nil".

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]