Re: [PATCH] emacs: Improved printing support.

[David Edmondson <dme@dme.org>]

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

On Mon, 16 Jan 2012 16:27:06 -0500, Aaron Ecay <aaronecay@gmail.com> wrote:
> > +;;
> 
> The above is just a stray comment line, right?

I tend to use them as spacers, but, sure.

> > +(defun notmuch-show-with-message-as-text (fn)
> > +  "Apply function `fn' to a text representation of the current
> > +message."
> 
> I think the docstring should say what agrs get passed to ‘fn’.  Also,
> function arguments should be referred to with capital letters, and the
> first line should be a complete sentence.  I’d recommend “Apply FN to
> (...the rest unchanged)” for brevity in the first line.
> 
> (For anyone who is curious, these and other conventions are documented
> in section D.6 of the Elisp manual.)

Will fix.
> > +		      (concat "Tags: "
> > +			      (mapconcat '(lambda (tag) tag) tags ", ") "\n")
> 
> #'identity instead of the lambda

/me slaps head. What was I thinking?

> > +	 (all (buffer-substring (notmuch-show-message-top)
> > +				(notmuch-show-message-bottom)))
> > +
> > +	 (file (make-temp-file "notmuch")))
> > +    (with-temp-file file
> > +      (insert all)
> > +      (indent-rigidly (point-min) (point-max) (- depth))
> > +      ;; Remove the original header.
> > +      (goto-char (point-min))
> > +      (re-search-forward "^$" (point-max) nil)
> > +      (delete-region (point-min) (point))
> > +      (insert header))
> > +    (funcall fn file (notmuch-show-get-message-properties))
> > +    (delete-file file)))
> 
> Why does this function write to a file?  It seems that, of the print
> methods, two (ps-print and lpr) don’t use the file at all.  One
> (ps-print/evince) creates yet another file, without depending on the
> first.  The other muttprint functions do use the file.  So:
> - This function would be more general if it didn’t handle file-writing
>   itself, but rather let each consumer of text-ified messages handle
>   that if needed.
> - It would be cleaner if print backends that don’t crucially depend on
>   the existence of a file didn’t create one at all.  If muttprint can
>   accept a message on stdin (it looks from the source like it can; it
>   gives me some error message about Iconv Perl modules when I try to run
>   it, so I can’t be sure), maybe writing to a file isn’t necessary at
>   all.

I'll rework it, thanks for the review!

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