Re: M4 support take#2

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Brad Knotwell <bknotwell@yahoo.com> writes:

> Given the code review from earlier, I've added a second file with the
> requested changes.

Thank you. Some minor comments follow.

> (defconst org-babel-header-args:m4
>   '((:cmd-line . :any)
>     (:quote . :any)
>     (:unquote . :any)
>     (:list-start . :any)
>     (:list-end . :any)
>     (:prefix-builtins))

Missing allowed type for last header. Maybe :any ?

> (defun org-babel--m4-prefix (params)
>   "Prefix m4_ if :prefix-builtins is set"
>   (if (assq :prefix-builtins params) "m4_" ""))
>
> (defun org-babel--m4-changequote (params)
>   "Declare quoting behavior if start-quote and end-quote are set.  Otherwise, return an empty string."

The line is too long. The second sentence should go onto another line.

>   (let ((prefix (org-babel--m4-prefix params))
> 	(start-quote (cdr (assq :quote params)))
> 	(end-quote (cdr (assq :unquote params))))
>     (if (and start-quote end-quote) (format "%schangequote(%s,%s)%sdnl\n" prefix start-quote end-quote prefix) "")))

See above.

> (defun org-babel--variable-assignment:m4_generic (params varname values)
>   "Build the simple macro definitions prepended to the script body."
>   (let ((prefix (org-babel--m4-prefix params)))
> 	(format "%sdefine(%s,%s)%sdnl\n" prefix varname values prefix)))

The (format ...) is not correctly indented.

> (defun org-babel--variable-assignment:m4_list (params varname values)
>   "Build the complex macro definitions prepended to the script body."
>   (let ((prefix (org-babel--m4-prefix params))
> 	(list-start (or (cdr (assq :list-start params)) "["))
> 	(list-end (or (cdr (assq :list-end params)) "]")))
>     (format "%sdefine(%s,%s%s%s)%sdnl\n" prefix varname list-start
> 	    (mapconcat
> 	     (lambda (value)
> 	       ;; value could be a numeric table entry as well as a string
> 	       (if (= (length value) 1) (format "%s" (car value))
> 		 (concat list-start (mapconcat (lambda (x) (format "%s" x)) value ",")
> 			 list-end))) values ",") list-end prefix)))

The line is too long. `values' should be below (lambda ...), so does
",". `list-end' and `prefix' should be below "%sdefine..."

> (defun org-babel--variable-assignments:m4 (params varnames values)
>   "Internal helper that converts parameters to m4 definitions."
>   (pcase values
>     (`(,_ . ,_) (org-babel--variable-assignment:m4_list params varnames values))
>     (_ (org-babel--variable-assignment:m4_generic params varnames values))))
>
> (defun org-babel-variable-assignments:m4 (params)
>   "Interface function that converts parameters to m4 definitions."
>   (concat (org-babel--m4-changequote params)
> 	  (apply #'concat (mapcar (lambda (pair) (org-babel--variable-assignments:m4
> 						  params (car pair) (cdr pair)))
> 				  (org-babel--get-vars params)))))

(mapcar ...) should be below #'concat.

> ;; Required to make tangling work
> ;; The final "\n" is needed as GNU m4 errors out if a file doesn't end in a newline.
> (defun org-babel-expand-body:m4 (body params)
>   "Expand BODY according to PARAMS, return the expanded body."
>   (concat (org-babel-variable-assignments:m4 params) body "\n"))
>
> (defun org-babel-execute:m4 (body params)
>   "Execute a block of m4 code with Org Babel.
> BODY is the source inside a m4 source block and PARAMS is an
> association list over the source block configurations.  This
> function is called by `org-babel-execute-src-block'."
>   (message "executing m4 source code block")
>   (let* ((result-params (cdr (assq :result-params params)))
>          (cmd-line (cdr (assq :cmd-line params)))
> 	 (prefix-builtins (assq :prefix-builtins params))
> 	 (code-file (let ((file (org-babel-temp-file "m4-")))
>                       (with-temp-file file
> 			(insert (org-babel-expand-body:m4 body params) file)) file))

Last `file' should be below (let ((file ...))).

> 	 (stdin (let ((stdin (cdr (assq :stdin params))))
> 		   (when stdin
> 		     (let ((tmp (org-babel-temp-file "m4-stdin-"))
> 			   (res (org-babel-ref-resolve stdin)))
> 		       (with-temp-file tmp
> 			 (insert res))
> 		       tmp))))
>          (cmd (mapconcat #'identity
> 			 (remq nil
> 			       (list org-babel-m4-command
> 				     cmd-line (if prefix-builtins "-P") "<" code-file))

(and prefix-builtins "-P")

"<" and `code-file' should go below `#'identity'.

I cannot remember: do you plan to have it integrated into Org proper? If
so, have you started the process of signing FSF papers?

Regards,

-- 
Nicolas Goaziou