Re: ob-sed

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Bjarte Johansen <bjarte.johansen@infomedia.uib.no> writes:

> I had loosely based it on my own ob-sparql and ob-awk, I saw that
> there was a remnant of ob-sparql left in there. Here is an updated
> version.

Thank you. Some comments follow.

> ;;; ob-sed.el --- org-babel functions for sed scripts
>
> ;; Copyright (C) 2015 Bjarte Johansen

You need to change the copyright to Free Software Foundation, Inc.
>
> ;; Author: Bjarte Johansen
> ;; Keywords: literate programming, reproducible research
> ;; Version: 0.1.0

You will need to add "This file is part of GNU Emacs."

> ;; Provides a way to evaluate sed scripts in org-mode.

org-mode -> Org mode

> (defvar org-babel-sed-command "sed")

Missing docstring.

> (defun org-babel-execute:sed (body params)
>   "Execute a block of sed code with org-babel.  This function is
> called by `org-babel-execute-src-block'"

org-babel -> Org Babel

"This function is" should be moved to a new line, not on the summary
line.  BODY and PARAMS ought to be explained.

>   (message "executing sed source code block")
>   (let* ((result-params (cdr (assoc :result-params params)))

`assoc' -> `assq' (same goes for other occurrences)

>          (cmd-line (cdr (assoc :cmd-line params)))
>          (in-file (cdr (assoc :in-file params)))

:cmd-line and :in-file look like sed-specific header arguments. If
that's correct, you should create a defconst,
`org-babel-header-args:sed' and list them here, probably with :any
value.

>          (cmd (mapconcat #'identity (remove nil (list org-babel-sed-command
> 						      "-f" code-file
> 						      cmd-line
> 						      in-file))
> 			 " ")))

`remove' -> `remq'

Bonus points for tests, too. Also, "org.texi" needs to be updated.


Regards,

-- 
Nicolas Goaziou