Date: Sat, 5 Apr 2014 20:58:20 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Mark Walters <markwalters1009@gmail.com>
Subject: Re: [PATCH 1/5] emacs: hello: add helper functions for saved-searches
Message-ID: <20140406005820.GD15472@mit.edu>
References: <1396733065-32602-1-git-send-email-markwalters1009@gmail.com>
	<1396733065-32602-2-git-send-email-markwalters1009@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1396733065-32602-2-git-send-email-markwalters1009@gmail.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Mark Walters on Apr 05 at 10:24 pm:
> Add helper functions to for saved searches to ease the transition to
> the new plist form while maintaining backwards compatibility. They
> will be used in the next patch.
> ---
>  emacs/notmuch-hello.el |   39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/emacs/notmuch-hello.el b/emacs/notmuch-hello.el
> index e325cd3..0b9ed16 100644
> --- a/emacs/notmuch-hello.el
> +++ b/emacs/notmuch-hello.el
> @@ -269,6 +269,45 @@ (defun notmuch-hello-search (&optional search)
>        (add-to-history 'notmuch-search-history search)))
>    (notmuch-search search notmuch-search-oldest-first))
>  
> +(defun notmuch-saved-search-get (saved-search field)
> +  "Get FIELD from SAVED-SEARCH.
> +
> +In the new style saved-search (a plist) this is just plist-get

It won't be "new style" once this has been in for a while.  Perhaps
"If SAVED-SEARCH is a plist, this is just `plist-get', but for
backwards compatibility, ..."

> +but, for backwards compatibility, this deals with the two
> +old-style forms: cons cells (NAME . QUERY) and lists (NAME QUERY
> +COUNT-QUERY)."
> +  (cond
> +   ((plist-get saved-search :name)

Rather than depending on :name, maybe this should be a more generic
plist test like (keywordp (car saved-search))?

> +    (plist-get saved-search field))
> +   ;; It is not a plist so it is an old-style entry.
> +   ((consp (cdr saved-search)) ;; It is a list (NAME QUERY COUNT-QUERY)
> +    (case field
> +      (:name (car saved-search))

Use `first' for consistency with the other case cases?

> +      (:query (second saved-search))
> +      (:count-query (third saved-search))
> +      (t nil)))
> +   (t  ;; It is a cons-cell (NAME . QUERY)
> +    (case field
> +      (:name (car saved-search))
> +      (:query (cdr saved-search))
> +      (t nil)))))
> +
> +(defun notmuch-hello-saved-search-to-plist (saved-search)
> +  "Convert a saved-search variable into plist form.

This takes a value, not a variable.  But it could be more succinct and
more accurate: "Return a copy of SAVED-SEARCH in plist form."

> +
> +The new style saved search is just a plist, but for backwards

Same comment about "new style".

> +compatatibility we use this function to give them in
> +plist-form. In all cases a new copy is returned so it is safe to

Grammar error?

> +modify the returned value."
> +  (if (and (listp (cdr saved-search)) (plist-member saved-search :name))
> +      (copy-seq saved-search)
> +    (let ((fields (list :name :query :count-query))
> +	  (plist-search))

Personally I prefer to either explicitly initialize nil variables or
to list them without the parenthesis at all (for some reason my brain
automatically reads this as an application), but if you prefer this,
that's fine, too.

> +      (dolist (field fields plist-search)
> +	(let ((string (notmuch-saved-search-get saved-search field)))
> +	  (when string
> +	    (setq plist-search (append plist-search (list field string)))))))))
> +
>  (defun notmuch-hello-add-saved-search (widget)
>    (interactive)
>    (let ((search (widget-value