Re: [O] [PATCH] ob-vala.el: Add Vala support to Babel

[Nicolas Goaziou]

Hello,

Christian Garbs <address@hidden> writes:

> * ob-vala.el: Add support for the Vala language to Babel.

Thank you. Comments follow.

> +;; file extension

;; File extension.

> +;; header arguments empty by default

;; Header argument empty by default.

> +(defvar org-babel-vala-compiler "valac"
> +  "Command used to compile a Vala source code file into an
> +executable.")

The first line of a docstring should be a full sentence.

Also, shouldn't this be a defcustom?

> +(defun org-babel-expand-body:vala (body params &optional processed-params)
> +  "Expand BODY: does nothing, returns original BODY while ignoring PARAMS."
> +  body ;; TODO: expand params?
> +  )
> +
> +;; This is the main function which is called to evaluate a code
> +;; block.
> +;;
> +;; - run Vala compiler and create a binary in a temporary file
> +;;   - compiler/linker flags can be set via :flags header argument
> +;; - if compilation succeeded, run the binary
> +;;   - commandline parameters to the binary can be set via :cmdline
> +;;     header argument
> +;;   - stdout will be parsed as RESULT (control via :result-params
> +;;     header argument)
> +;;
> +;; There is no session support because Vala is a compiled language.
> +;;
> +;; This function is heavily based on ob-C.el
> +(defun org-babel-execute:vala (body params)
> +  "Execute a block of Vala code with org-babel.

"org-babel" -> "Babel"

> +This function is called by `org-babel-execute-src-block'"
> +  (message "executing Vala source code block")
> +  (let* ((tmp-src-file (org-babel-temp-file
> +                     "vala-src-"
> +                     ".vala"))
> +         (tmp-bin-file (org-babel-temp-file "vala-bin-" org-babel-exeext))
> +         (cmdline (cdr (assoc :cmdline params)))
> +         (flags (cdr (assoc :flags params)))

`assoc' -> `assq'

> +         (full-body (org-babel-expand-body:vala body params))
> +         (compile
> +       (progn
> +         (with-temp-file tmp-src-file (insert full-body))
> +         (org-babel-eval
> +          (format "%s %s -o %s %s"
> +                  org-babel-vala-compiler
> +                  (mapconcat 'identity

'identity -> #'identity

> +                             (if (listp flags) flags (list flags)) " ")
> +                  (org-babel-process-file-name tmp-bin-file)
> +                  (org-babel-process-file-name tmp-src-file)) "")))
> +      )

There should be no dangling parenthesis. Please move it at the end of
the line above.

> +    (if (file-executable-p tmp-bin-file)
> +     (let ((results
> +            (org-trim
> +             (org-babel-eval
> +              (concat tmp-bin-file (if cmdline (concat " " cmdline) "")) 
> ""))))
> +       (org-babel-reassemble-table
> +        (org-babel-result-cond (cdr (assoc :result-params params))
> +          (org-babel-read results)
> +          (let ((tmp-file (org-babel-temp-file "vala-")))
> +            (with-temp-file tmp-file (insert results))
> +            (org-babel-import-elisp-from-file tmp-file)))
> +        (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)))
> +        ))
> +      )))

See above.

Also, it looks like you are using a one-armed `if'. In this case,
I suggest using (when ...) instead.

> +(defun org-babel-prep-session:vala (session params)
> +  "This function does nothing as Vala is a compiled language with no
> +support for sessions"
> +  (error "Vala is a compiled language -- no support for sessions"))

See remark about first line in a docstirng.

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

Ditto.

> +(defun org-babel-vala-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."
> +  )

The body of the function is missing.

Could you send an updated patch?

Regards,

-- 
Nicolas Goaziou