To: Adam Wolfe Gordon <awg+notmuch@xvx.ca>, notmuch@notmuchmail.org,
 awg@xvx.ca
Subject: Re: [PATCH 4/4] emacs: Use the new JSON reply format.
In-Reply-To: <1326009162-19524-5-git-send-email-awg+notmuch@xvx.ca>
References: <1326009162-19524-1-git-send-email-awg+notmuch@xvx.ca>
	<1326009162-19524-5-git-send-email-awg+notmuch@xvx.ca>
User-Agent: Notmuch/0.10.2+151~gbf1dc2b (http://notmuchmail.org)
	Emacs/24.0.92.1 (x86_64-pc-linux-gnu)
From: David Edmondson <dme@dme.org>
Date: Mon, 09 Jan 2012 08:50:00 +0000
Message-ID: <cun62gl1cqv.fsf@hotblack-desiato.hh.sledj.net>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Precedence: list

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On Sun,  8 Jan 2012 00:52:42 -0700, Adam Wolfe Gordon <awg+notmuch@xvx.ca> =
wrote:
> +(defun w3m-region (start end)) ;; From `w3m.el'.
> +(defun notmuch-mua-quote-part (part)
> +  (with-temp-buffer
> +    (insert part)
> +    (message-mode)
> +    (fill-region (point-min) (point-max))
> +    (goto-char (point-min))
> +    (perform-replace "^" "> " nil t nil)
> +    (set-buffer-modified-p nil)
> +    (buffer-substring (point-min) (point-max))))

Couldn't all of this be done directly in the reply buffer?

> +(defun notmuch-mua-parse-html-part (part)
> +  (with-temp-buffer
> +    (insert part)
> +    (w3m-region (point-min) (point-max))
> +    (set-buffer-modified-p nil)
> +    (buffer-substring (point-min) (point-max))))

Blank lines between functions are the house style (as much as there is
such a thing).

Using w3m means that you should `require' it. What happens when a user
doesn't have it? (Either the elisp or the command.)

>  (defun notmuch-mua-reply (query-string &optional sender)
> -  (let (headers
> +  (let (reply
> +	original
> +	headers
>  	body
> -	(args '("reply")))
> +	(args '("reply" "--format=3Djson")))

Initialised variables are generally shown before uninitialised. I know
that you didn't do this 'wrong' and that it's an unrelated change, but
it should be fixed. (To the people who say that should be a separate
patch I say 'meh' - life is too short.)

>      (if notmuch-show-process-crypto
>  	(setq args (append args '("--decrypt"))))
>      (setq args (append args (list query-string)))
> -    ;; This make assumptions about the output of `notmuch reply', but
> -    ;; really only that the headers come first followed by a blank
> -    ;; line and then the body.
> +    ;; Get the reply object as JSON, and parse it into an elisp object.
>      (with-temp-buffer
>        (apply 'call-process (append (list notmuch-command nil (list t t) =
nil) args))
>        (goto-char (point-min))
> -      (if (re-search-forward "^$" nil t)
> -	  (save-excursion
> -	    (save-restriction
> -	      (narrow-to-region (point-min) (point))
> -	      (goto-char (point-min))
> -	      (setq headers (mail-header-extract)))))
> -      (forward-line 1)
> -      (setq body (buffer-substring (point) (point-max))))
> +      (setq reply (aref (json-read) 0)))
> +
> +    ;; Get the list of headers
> +    (setq headers (cdr (assq 'headers (assq 'reply reply))))
> +    ;; Construct the body of the reply.
> +    (setq original (cdr (assq 'original reply)))

The scope of (at least) `headers' and `original' could be tightened by
moving them down to the following `let' (and converting it to `let*').

> +
> +    ;; Start with the prelude, based on the headers of the original mess=
age.
> +    (let ((original-headers (cdr (assq 'headers original))))
> +      (setq body (format "On %s, %s wrote:\n"
> +			 (cdr (assq 'date original-headers))
> +			 (cdr (assq 'from original-headers)))))
> +
> +    ;; Extract the body parts and construct a reasonable quoted body.
> +    (let* ((body-parts (cdr (assq 'body original)))
> +	   (find-parts (lambda (type) (delq nil (mapcar (lambda (part)
> +							  (if (string=3D (cdr (assq 'content-type part)) type)
> +							      (cdr (assq 'content part))))
> +							body-parts))))

`find-parts' might be useful in other places. Maybe add it to notmuch-lib.e=
l?

> +	   (plain-parts (apply find-parts '("text/plain")))
> +	   (html-parts (apply find-parts '("text/html"))))
> +=20=20=20=20=20=20
> +      (if (not (null plain-parts))
> +	  (mapc (lambda (part) (setq body (concat body (notmuch-mua-quote-part =
part)))) plain-parts)
> +	(mapc (lambda (part) (setq body (concat body (notmuch-mua-quote-part (n=
otmuch-mua-parse-html-part part))))) html-parts)))

If you have an 'else' clause, why test '(if (not ..' ?

> +    (setq body (concat body "\n"))
> +=09

If it already ends with a carriage return, why do this?

--=-=-=
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAk8KqjgACgkQaezQq/BJZRYKjgCdFRk68a66Bwlk5yRAfXGvAgtG
/akAn1yNHIX6CaeXPLKMRtwiSHBvYu8+
=/FA4
-----END PGP SIGNATURE-----
--=-=-=--