From: Nicolas Goaziou <mail@nicolasgoaziou.fr>
To: Hex <hex@neg9.org>
Cc: Emacs-orgmode@gnu.org
Subject: Re: contribution: ob-php
Date: Tue, 13 Jan 2015 12:00:03 +0100 [thread overview]
Message-ID: <87twzvc5e4.fsf@nicolasgoaziou.fr> (raw)
In-Reply-To: <20150112121032.e352cf20e00a217e4bc8a956@neg9.org> (hex@neg9.org's message of "Mon, 12 Jan 2015 12:10:32 -0800")
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
prev parent reply other threads:[~2015-01-13 10:59 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-01-12 20:10 contribution: ob-php Hex
2015-01-13 11:00 ` Nicolas Goaziou [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87twzvc5e4.fsf@nicolasgoaziou.fr \
--to=mail@nicolasgoaziou.fr \
--cc=Emacs-orgmode@gnu.org \
--cc=hex@neg9.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).