Re: contribution: ob-php

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Hex <hex@neg9.org> writes:

> I use org mode for code review quite a bit. It's awesome to be able to
> throw snippets of code in a doc and be able to verify fixes within the
> doc. I noticed there's no PHP mode (and totally understand why seeing
> as how "literate programming" is a keyword ;) so I wrote one.
>
> Elsip isn't exactly a language I hit a lot, so it might be pretty
> rough. I added the following files:
>
> lisp/ob-php.el
> testing/examples/ob-php-test.org
> testing/lisp/test-ob-php.el

Thanks for your work.

However, we can only apply it on master branch once FSF assignment is
completed. It can take a couple of weeks, depending on your location.

Also, please use git format-patch with a proper commit message when
providing a patch.

Some comments follow.

> +;; Copyright (C) 2014 Josh Dukes

This will need to be the "Free Software Foundation, Inc."

> +;; This function expands the body of a source code block by doing
> +;; things like prepending argument definitions to the body, it is
> +;; be called by the `org-babel-execute:php' function below.
> +(defun org-babel-expand-body:php (body params &optional processed-params)
> +  "Expand BODY according to PARAMS, return the expanded body."

You should give the type of BODY (probably a string) and PARAMS (a
plist), and describe PROCESSED-PARAMS.

> +  (if (or (car (assoc :expand params)) nil)

  (if (car (assq :expand params)) ...)

Also, it is probably `cdr' instead of `car'.
  
> +      (let ((full-body
> +	     (org-babel-expand-body:generic
> +	      body
> +	      params
> +	      (org-babel-variable-assignments:php params))))
> +	(concat "<?php\n"
> +		full-body
> +		"\n?>"))

IMO, this is unnecessarily convoluted

  (format "<?php\n%s\n?>"
          (org-babel-expand-body:generic
           ...))

> +    (let ((vars
> +	   (concat "<?php\n"
> +		   (mapconcat
> +		    #'identity
> +		    (org-babel-variable-assignments:php params)
> +		    "\n")
> +		   "\n?>")))
> +      (concat vars body))))

Ditto.

(concat "<?php\n"
        (mapconcat ...)
        "\n?>"
        body)

> +(defun org-babel-execute:php (body params)

Missing docstring.

> +  (let* ((session (cdr (assoc :session params)))
> +	 (flags (or (cdr (assoc :flags params)) ""))

  `assoc' -> `assq'

> +	 (src-file (org-babel-temp-file "php-src-"))
> +	 (full-body (org-babel-expand-body:php body params))
> +	 (session (org-babel-php-initiate-session session))
> +	 (results
> +	  (progn
> +	    (with-temp-file src-file (insert full-body))
> +	    (org-babel-eval
> +	     (concat org-babel-php-command
> +		     " " flags " " src-file) ""))))
> +    (progn

`progn' is implicit around the body of `let' (or `let*'). You can remove
it.

> +      (org-babel-reassemble-table
> +       (org-babel-result-cond (cdr (assoc :result-params params))
> +	 (org-babel-read results)
> +         (let ((tmp-file (org-babel-temp-file "c-")))
> +           (with-temp-file tmp-file (insert results))
> +           (org-babel-import-elisp-from-file tmp-file)))

Is the temporary file necessary? `with-temp-buffer' is cheaper. Use it
if possible.

> +       (org-babel-pick-name
> +        (cdr (assoc :colname-names params)) (cdr (assoc :colnames params)))
> +       (org-babel-pick-name
> +        (cdr (assoc :rowname-names params)) (cdr (assoc :rownames
> params)))))))

  `assoc' -> `assq'

> +
> +(defun org-babel-variable-assignments:php (params)
> +  "Return a list of PHP statements assigning the block's variables."

You need to describe PARAMS.

> +  (mapcar
> +   (lambda (pair)
> +     (format "$%s=%s;"
> +	     (car pair)
> +	     (org-babel-php-var-to-php (cdr pair))))
> +   (mapcar #'cdr (org-babel-get-header params :var))))

Nitpick: can't you do it with a single `mapcar'?

> +(defun org-babel-php-var-to-php (var)
> +  "Convert an elisp var into a string of php source code
> +specifying a var of the same value."

First sentence of docstring has to fit on a single line.

> +  (if (listp var)
> +      (concat "Array(" (mapconcat #'org-babel-php-var-to-php var ",") ")")
> +  (format "%S" var)))
> +
> +(defun org-babel-php-table-or-string (results)
> +  "If the results look like a table, then convert them into an
> +Emacs-lisp table, otherwise return the results as a string."

See above.

> +++ b/testing/examples/ob-php-test.org

I know Babel usually relies on an external Org file for its tests, but
this is a pain to debug when one fails.

It is not terribly important, but write self-contained tests if you can,
i.e., using `org-test-with-temp-text'.


> diff --git a/testing/lisp/test-ob-php.el b/testing/lisp/test-ob-php.el
> new file mode 100644
> index 0000000..47a1169
> --- /dev/null
> +++ b/testing/lisp/test-ob-php.el

[...]

> +(ert-deftest ob-php/assert ()
> +  (should t))

You can remove that.


Regards,

-- 
Nicolas Goaziou