Re: [O] [PATCH] expose nrepl's timeout setting in ob-clojure.el

[Nicolas Goaziou]

Hello,

Thank you for the patch.

Frederick Giasson <address@hidden> writes:

> -;;; ob-clojure.el --- Babel Functions for Clojure    -*- lexical-binding: t; 
> -*-
> +;;; ob-clojure.el --- org-babel functions for clojure evaluation

Your patch reverts a change introduced in development version. Could you
rebase it on top of latest Org first?

>  
>  ;; Copyright (C) 2009-2016 Free Software Foundation, Inc.
>  
> @@ -55,6 +55,7 @@
>  
>  (defvar org-babel-default-header-args:clojure '())
>  (defvar org-babel-header-args:clojure '((package . :any)))
> +(defvar org-babel-clojure-nrepl-timeout 10)

Would it make sense to turn it into a defcustom instead? If so, this
should be added in etc/ORG-NEWS file. Bonus points if Worg documentation
about this back-end 
>  
>  (defcustom org-babel-clojure-backend
>    (cond ((featurep 'cider) 'cider)
> @@ -67,7 +68,7 @@
>  
>  (defun org-babel-expand-body:clojure (body params)
>    "Expand BODY according to PARAMS, return the expanded body."
> -  (let* ((vars (org-babel--get-vars params))
> +  (let* ((vars (mapcar #'cdr (org-babel-get-header params :var)))

You are also reverting a previous change here.

>        (result-params (cdr (assoc :result-params params)))
>        (print-level nil) (print-length nil)
>        (body (org-babel-trim
> @@ -94,8 +95,9 @@
>         (let ((result-params (cdr (assoc :result-params params))))
>        (setq result
>              (nrepl-dict-get
> -             (nrepl-sync-request:eval
> -              expanded (cider-current-connection) (cider-current-session))
> +             (let ((nrepl-sync-request-timeout 
> org-babel-clojure-nrepl-timeout))
> +               (nrepl-sync-request:eval
> +                expanded (cider-current-connection) (cider-current-session)))

It seems you need to define `nrepl-sync-request-timeout' as
a dynamically scoped variable:

  (defvar nrepl-sync-request-timeout)

Indeed "ob-clojure.el" uses lexical binding, even though you disabled it
in the first line of your patch ;)

Eventually, could you add an appropriate commit message for this patch?
Something like

  ob-clojure: Make REPL timeout configurable

  * lisp/ob-clojure.el (org-babel-clojure-nrepl-timeout):  New variable.
  (org-babel-expand-body:clojure): Use new variable.

  ... explanations about the motivation of the change...


Regards,

-- 
Nicolas Goaziou