From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nicolas Goaziou Subject: Re: contribution: ob-php Date: Tue, 13 Jan 2015 12:00:03 +0100 Message-ID: <87twzvc5e4.fsf@nicolasgoaziou.fr> References: <20150112121032.e352cf20e00a217e4bc8a956@neg9.org> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:35853) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAzBi-0003De-Lw for Emacs-orgmode@gnu.org; Tue, 13 Jan 2015 05:59:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YAzBd-0004D3-FV for Emacs-orgmode@gnu.org; Tue, 13 Jan 2015 05:59:06 -0500 Received: from relay3-d.mail.gandi.net ([2001:4b98:c:538::195]:45024) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YAzBd-0004Cq-7Y for Emacs-orgmode@gnu.org; Tue, 13 Jan 2015 05:59:01 -0500 In-Reply-To: <20150112121032.e352cf20e00a217e4bc8a956@neg9.org> (hex@neg9.org's message of "Mon, 12 Jan 2015 12:10:32 -0800") List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: Hex Cc: Emacs-orgmode@gnu.org Hello, Hex 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 " + full-body > + "\n?>")) IMO, this is unnecessarily convoluted (format "" (org-babel-expand-body:generic ...)) > + (let ((vars > + (concat " + (mapconcat > + #'identity > + (org-babel-variable-assignments:php params) > + "\n") > + "\n?>"))) > + (concat vars body)))) Ditto. (concat "" 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