Re: [O] [PATCH] migrate ob-clojure initiate session code from ob-clojure-literate.el into ob-clojure.el

[Nicolas Goaziou]

Hello,

stardiviner <address@hidden> writes:

> Those code belongs to ob-clojure.el.

Thank you. Comments follow.

> From 7306147a55ea29be7a685cd7a92dc158612dfccd Mon Sep 17 00:00:00 2001
> From: stardiviner <address@hidden>
> Date: Thu, 19 Apr 2018 18:16:27 +0800
> Subject: [PATCH] * ob-clojure.el: support `org-babel-initiate-session' to
>  initialize.

  ob-clojure.el: Support `org-babel-initiate-session' to initialize.

(no leading star, capitalization)

> Migrate from ob-clojure-literate.el into ob-clojure.el.

You need to expound the commit message. In particular, you need to
specify which functions are new, which are modified.

> +(defun org-babel-clojure-initiate-session (&optional session _params)
> +  "Initiate a session named SESSION according to PARAMS."
> +  (when (and session (not (string= session "none")))
> +    (save-window-excursion
> +      (unless (org-babel-comint-buffer-livep session)

You can merge the `unless' within the `cond', e.g.,

(cond
 ((org-babel-comint-buffer-livep session) nil)
 ...)

> +        ;; CIDER jack-in to the Clojure project directory.
> +        (cond
> +         ((eq org-babel-clojure-backend 'cider)
> +          (require 'cider)
> +          (let ((session-buffer (save-window-excursion
> +                                  (cider-jack-in t)
> +                                  (current-buffer))))
> +            (if (org-babel-comint-buffer-livep session-buffer)
> +                (progn (sit-for .25) session-buffer))))
> +         ((eq org-babel-clojure-backend 'slime)
> +          (error "Session evaluation with SLIME is not supported"))
> +         (t
> +          (error "Session initiate failed")))
> +        )
> +      (get-buffer session)
> +      )))

No dangling parens, please.

> +(defun org-babel-prep-session:clojure (session params)
> +  "Prepare SESSION according to the header arguments specified in PARAMS."
> +  (let* ((session (org-babel-clojure-initiate-session session))
> +         (var-lines (org-babel-variable-assignments:clojure params)))

No need for `let*'. `let' is sufficient.

> +    (when session
> +      (org-babel-comint-in-buffer session
> +        (mapc (lambda (var)
> +                (insert var) (comint-send-input nil t)
> +             (org-babel-comint-wait-for-output session)
> +             (sit-for .1) (goto-char (point-max))) var-lines)))
> +    session))

`mapc' + `lambda' -> `dolist', e.g:

    (defun org-babel-prep-session:clojure (session params)
      "Prepare SESSION according to the header arguments specified in PARAMS."
      (let ((session (org-babel-clojure-initiate-session session))
            (var-lines (org-babel-variable-assignments:clojure params)))
        (when session
          (org-babel-comint-in-buffer session
            (dolist (var var-lines)
              (insert var)
              (comint-send-input nil t)
              (org-babel-comint-wait-for-output session)
              (sit-for .1)
              (goto-char (point-max)))))
        session))

> +(defun org-babel-clojure-var-to-clojure (var)
> +  "Convert src block's `VAR' to Clojure variable."

VAR, not `VAR'

> +  (if (listp var)
> +      (replace-regexp-in-string "(" "'(" var)
> +    (cond

You can merge the THEN part of the `if' in the `cond'.

> +     ((stringp var)
> +      ;; wrap org-babel passed in header argument value with quote in 
> Clojure.

;; Wrap Babel passed-in header argument value with quotes in Cojure.

> +      (format "\"%s\"" var))

  (format "%S" var)
  
> +     (t
> +      (format "%s" var))))
> +  )

Dangling parens.

> +(defun org-babel-variable-assignments:clojure (params)
> +  "Return a list of Clojure statements assigning the block's variables in 
> `PARAMS'."

PARAMS, not `PARAMS'.

> +  (mapcar
> +   (lambda (pair)
> +     (format "(def %s %s)"
> +             (car pair)
> +             (org-babel-clojure-var-to-clojure (cdr pair))))
> +   (org-babel--get-vars params)))

Would it make sense to add a few tests for this?

Regards,

-- 
Nicolas Goaziou