Re: [O] [PATCH] add :session support for ob-js.el

[Nicolas Goaziou]

Hello,

stardiviner <address@hidden> writes:

> I added org-mode babel ob-js.el header argument :session.
>
> Following packages :session are supported:
>
> - skewer-mode
>
> - js-comint
>
> - Indium

I don't know any of these, but here come some comments about the code.

>  then create.  Return the initialized session."
>    (unless (string= session "none")
>      (cond

Nitpick: You can merge the `unless' into the `cond'.

> -     ((string= "*skewer-repl*" session)
> +     ((and (string= "js-comint" org-babel-js-cmd) ; `js-comint'
> +        (string= "*Javascript REPL*" session))
> +      (require 'js-comint)
> +      (let ((session-buffer "*Javascript REPL*"))
> +     (if (and (org-babel-comint-buffer-livep (get-buffer session-buffer))
> +              (comint-check-proc session-buffer))
> +         session-buffer
> +       (call-interactively 'run-js)

The `run-js' probably needs to be declared at the beginning of the file.

> +                  (cond
> +                   ;; Indium Node
> +                   ((string= "*JS REPL*" session)
> +                    (require 'indium-repl)
> +                    (unless (get-buffer session)
> +                      (indium-run-node))

The function above needs to be declared, too.

> +                    (indium-eval full-body))

So does this one.

> +                   (t
> +                    (let ((session (org-babel-prep-session:js
> +                                    (cdr (assq :session params)) params)))
> +                      (nth 1
> +                           (org-babel-comint-with-output
> +                               (session (format "%S" org-babel-js-eoe) t 
> body)
> +                             (mapc   ; FIXME: stack on this scope when 
> `skewer-eval'

What does mean this FIXME?

> +                              (lambda (line)

Nitpick: `mapc' + `lambda' -> `dolist'

>      (cond
> +     ((string= "*skewer-repl*" session)
> +      (require 'skewer-repl)
> +      (let ((session-buffer (get-buffer "*skewer-repl*")))
> +     (if (and (org-babel-comint-buffer-livep (get-buffer session-buffer))
> +              (comint-check-proc session-buffer))
> +         session-buffer
> +       ;; start skewer REPL.
> +       (sit-for .5)

Why is this `sit-for' needed?

> +       (httpd-start)
> +       (run-skewer)

These functions need to be declared.

Regards,

-- 
Nicolas Goaziou