Re: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list

Re: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list

[Ihor Radchenko]

"Christopher M. Miles" <numbchild@gmail.com> writes:

> I bellowing example:
>
> #+begin_src org
> ,#+NAME: ob-clojure-table-test
> | a | b | c |
> |---+---+---|
> | 1 | 2 | 3 |
>
> ,#+NAME: ob-clojure-table-test-2
> | a | b | c |
> |---+---+---|
> | 1 | 2 | 3 |
>
> ,#+begin_src clojure :var kk=ob-clojure-table-test :var kk2=ob-clojure-table-test-2 :results output
> (println kk2)
> (println kk)
> ,#+end_src
>
> #+end_src
>
> Without this patch, it will report error "class java.lang.ClassCastException" from CIDER.

Bastien, could you please take a look? I was unable to setup clojure dev
environment on my machine for testing. So, I am not able to confirm if
the issue exists.

>  		 ;; Variables binding.
>  		 (if (null vars) (org-trim body)
> -		   (format "(let [%s]\n%s)"
> -			   (mapconcat
> -			    (lambda (var)
> -			      (format "%S %S" (car var) (cdr var)))
> -			    vars
> -			    "\n      ")
> -			   body))))))
> +                   ;; variable's value is a list from org-mode passed table or list.
> +		   (if (listp (cdr (car vars)))

This test is fishy. It only tests for the first variable assignment.
What if you have multiple vars some being tables and some not?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list

[Daniel Kraus]

[-- Attachment #1: Type: text/plain, Size: 1634 bytes --]

Hi!

Ihor Radchenko <yantar92@posteo.net> writes:

> "Christopher M. Miles" <numbchild@gmail.com> writes:
>> Without this patch, it will report error "class java.lang.ClassCastException" from CIDER.
>
> Bastien, could you please take a look? I was unable to setup clojure dev
> environment on my machine for testing. So, I am not able to confirm if
> the issue exists.

Just in case you want to play around with Clojure, installing babashka
(https://github.com/babashka/babashka) is a single binary and very simple to install.

>>  		 ;; Variables binding.
>>  		 (if (null vars) (org-trim body)
>> -		   (format "(let [%s]\n%s)"
>> -			   (mapconcat
>> -			    (lambda (var)
>> -			      (format "%S %S" (car var) (cdr var)))
>> -			    vars
>> -			    "\n      ")
>> -			   body))))))
>> +                   ;; variable's value is a list from org-mode passed table or list.
>> +		   (if (listp (cdr (car vars)))
>
> This test is fishy. It only tests for the first variable assignment.
> What if you have multiple vars some being tables and some not?

I looked at `ob-lisp.el` and simply always quoting seems to be working.
See attached patch and here's my test (based from Chrisophers example):

--cut--

#+NAME: ob-clojure-table-test
| a | b | c |
|---+---+---|
| 1 | 2 | 3 |

#+NAME: ob-clojure-table-test-2
| a | b | c |
|---+---+---|
| 1 | 2 | 3 |
| 4 | 5 | 6 |

#+begin_src clojure :var v1=42 :var v2="foobar" :var v3=ob-clojure-table-test :var v4=ob-clojure-table-test-2 :results output
(prn (+ v1 5))
(prn v2)
(prn v3)
(prn v4)
#+end_src

#+RESULTS:
: 47
: "foobar"
: ((1 2 3))
: ((1 2 3) (4 5 6))

--cut--

Cheers,
  Daniel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-ob-clojure.el-Fix-header-argument-var-binding.patch --]
[-- Type: text/x-patch, Size: 999 bytes --]

From 1a2fa382e7a5925d4b85d90f1fe4ac7c012f81a4 Mon Sep 17 00:00:00 2001
From: Daniel Kraus <daniel@kraus.my>
Date: Thu, 27 Oct 2022 16:04:02 +0200
Subject: [PATCH] lisp/ob-clojure.el: Fix header argument :var binding

* lisp/ob-clojure.el (org-babel-expand-body:clojure): Always quote
the variables passed from org-mode in clojure let binding.
When a variable is a table or list, it's value is is "(..data..)"
and without quotes, clojure would try to execute the first
value as a function.
---
 lisp/ob-clojure.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/ob-clojure.el b/lisp/ob-clojure.el
index 0649469b3..c76a3f013 100644
--- a/lisp/ob-clojure.el
+++ b/lisp/ob-clojure.el
@@ -132,7 +132,7 @@ or set the `:backend' header argument"))))
 		   (format "(let [%s]\n%s)"
 			   (mapconcat
 			    (lambda (var)
-			      (format "%S %S" (car var) (cdr var)))
+			      (format "%S '%S" (car var) (cdr var)))
 			    vars
 			    "\n      ")
 			   body))))))
-- 
2.38.1

Re: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list

[Ihor Radchenko]

Daniel Kraus <daniel@kraus.my> writes:

> Just in case you want to play around with Clojure, installing babashka
> (https://github.com/babashka/babashka) is a single binary and very simple to install.

Thanks!

> Subject: [PATCH] lisp/ob-clojure.el: Fix header argument :var binding
>
> * lisp/ob-clojure.el (org-babel-expand-body:clojure): Always quote
> the variables passed from org-mode in clojure let binding.
> When a variable is a table or list, it's value is is "(..data..)"

*is is

Otherwise, feel free to push.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list

[Bastien Guerry]

Hi Daniel,

Daniel Kraus <daniel@kraus.my> writes:

> Just in case you want to play around with Clojure, installing
> babashka (https://github.com/babashka/babashka) is a single binary
> and very simple to install.

For now `org-babel-clojure-backend' is nil.

I suggest to set it to babashka by default: babashka is stable and
well established now, it is by far the most efficient way to run
Clojure code.  Also, it is particularily suitable for Babel use.

What do you think?

-- 
 Bastien

Re: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list

[Daniel Kraus]

Hi!

Bastien Guerry <bzg@gnu.org> writes:
> For now `org-babel-clojure-backend' is nil.
>
> I suggest to set it to babashka by default: babashka is stable and
> well established now, it is by far the most efficient way to run
> Clojure code.  Also, it is particularily suitable for Babel use.
>
> What do you think?

Sounds good.
I would set it to (and (executable-find "bb") 'babashka) so it's still nil
when babashka is installed?

Or we could even test more available backends like

(cond
 ((executable-find "bb") 'babashka)
 ((executable-find "nbb") 'nbb)
 ((featurep 'cider) 'cider)
 ((featurep 'inf-clojure) 'inf-clojure)
 ((featurep 'slime) 'slime))

Or is that too much "magic" in that on some systems the default is bb
and in others it's cider etc?

Cheers,
  Daniel

Re: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list

[Bastien]

Hi Daniel,

Daniel Kraus <daniel@kraus.my> writes:

> I would set it to (and (executable-find "bb") 'babashka) so it's still nil
> when babashka is installed?

(You mean "not installed", right?)

> Or we could even test more available backends like
>
> (cond
>  ((executable-find "bb") 'babashka)
>  ((executable-find "nbb") 'nbb)
>  ((featurep 'cider) 'cider)
>  ((featurep 'inf-clojure) 'inf-clojure)
>  ((featurep 'slime) 'slime))
>
> Or is that too much "magic" in that on some systems the default is bb
> and in others it's cider etc?

I think this is acceptable magic: perhaps we should first check what
is done in other Babel languages and align with their level of magic
for guessing the correct executable -- but I suspect Clojure is a bit
special here, in that it has a lot of different options.

Thanks for taking care of this,

-- 
 Bastien

Re: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list

[Daniel Kraus]

Bastien <bzg@gnu.org> writes:

> Daniel Kraus <daniel@kraus.my> writes:
>> I would set it to (and (executable-find "bb") 'babashka) so it's still nil
>> when babashka is installed?
> (You mean "not installed", right?)

Of course.

>> Or we could even test more available backends like
>>
>> (cond
>>  ((executable-find "bb") 'babashka)
>>  ((executable-find "nbb") 'nbb)
>>  ((featurep 'cider) 'cider)
>>  ((featurep 'inf-clojure) 'inf-clojure)
>>  ((featurep 'slime) 'slime))
>>
>> Or is that too much "magic" in that on some systems the default is bb
>> and in others it's cider etc?
>
> I think this is acceptable magic: perhaps we should first check what
> is done in other Babel languages and align with their level of magic
> for guessing the correct executable -- but I suspect Clojure is a bit
> special here, in that it has a lot of different options.

I looked around a bit but couldn't find another babel package with a
similar problems, i.e. that some backends are completely different
and not just changing the name of the executable.

I think I'll go with the big `cond` above to auto-detect what's
installed. That's probably the best out-of-the-box experience.

Cheers,
  Daniel

Re: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list

[Bastien]

Daniel Kraus <daniel@kraus.my> writes:

> I think I'll go with the big `cond` above to auto-detect what's
> installed. That's probably the best out-of-the-box experience.

Indeed, thank you!

-- 
 Bastien

Auto detect ob-clojure backend (was: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list)

[Daniel Kraus]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]


Bastien <bzg@gnu.org> writes:

> Daniel Kraus <daniel@kraus.my> writes:
>
>> I think I'll go with the big `cond` above to auto-detect what's
>> installed. That's probably the best out-of-the-box experience.
> Indeed, thank you!

I would push the attached patch.
I'm not sure about the `:package-version` keyword in defcustom.
I want to say that the default value changed.
Should I do it this way? Is `(Org . "9.6")` correct as it's unreleased?

Thanks,
  Daniel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ob-clojure.el-Auto-detect-backend.patch --]
[-- Type: text/x-patch, Size: 1351 bytes --]

From bd68ece99ce411439db87cd19e44ffbc49677ae3 Mon Sep 17 00:00:00 2001
From: Daniel Kraus <daniel@kraus.my>
Date: Sat, 29 Oct 2022 23:20:06 +0200
Subject: [PATCH] ob-clojure.el: Auto detect backend

* lisp/ob-clojure.el (org-babel-clojure-backend): Set the backend
to an available cli program or elisp package.
---
 lisp/ob-clojure.el | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-clojure.el b/lisp/ob-clojure.el
index 5654d5208..25f1c78a6 100644
--- a/lisp/ob-clojure.el
+++ b/lisp/ob-clojure.el
@@ -76,9 +76,16 @@
 (defvar org-babel-default-header-args:clojurescript '())
 (defvar org-babel-header-args:clojurescript '((package . :any)))
 
-(defcustom org-babel-clojure-backend nil
+(defcustom org-babel-clojure-backend (cond
+                                      ((executable-find "bb") 'babashka)
+                                      ((executable-find "nbb") 'nbb)
+                                      ((featurep 'cider) 'cider)
+                                      ((featurep 'inf-clojure) 'inf-clojure)
+                                      ((featurep 'slime) 'slime)
+				      (t nil))
   "Backend used to evaluate Clojure code blocks."
   :group 'org-babel
+  :package-version '(Org . "9.6")
   :type '(choice
 	  (const :tag "inf-clojure" inf-clojure)
 	  (const :tag "cider" cider)
-- 
2.38.1

Re: Auto detect ob-clojure backend (was: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list)

[Ihor Radchenko]

Daniel Kraus <daniel@kraus.my> writes:

> Bastien <bzg@gnu.org> writes:
>
>> Daniel Kraus <daniel@kraus.my> writes:
>>
>>> I think I'll go with the big `cond` above to auto-detect what's
>>> installed. That's probably the best out-of-the-box experience.
>> Indeed, thank you!
>
> I would push the attached patch.

> +(defcustom org-babel-clojure-backend (cond
> +                                      ((executable-find "bb") 'babashka)
> +                                      ((executable-find "nbb") 'nbb)
> +                                      ((featurep 'cider) 'cider)
> +                                      ((featurep 'inf-clojure) 'inf-clojure)
> +                                      ((featurep 'slime) 'slime)
> +				      (t nil))

What if users have, say, cider installed and also babashka executable? 
Will it be expected to use babashka?

> I'm not sure about the `:package-version` keyword in defcustom.
> I want to say that the default value changed.
> Should I do it this way? Is `(Org . "9.6")` correct as it's unreleased?

Yes. 9.6 is the next release - our main branch is labeled Org 9.6-pre.
So, using 9.6 is correct.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Auto detect ob-clojure backend (was: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list)

[Daniel Kraus]

Ihor Radchenko <yantar92@posteo.net> writes:

> Daniel Kraus <daniel@kraus.my> writes:
>
>> +(defcustom org-babel-clojure-backend (cond
>> +                                      ((executable-find "bb") 'babashka)
>> +                                      ((executable-find "nbb") 'nbb)
>> +                                      ((featurep 'cider) 'cider)
>> +                                      ((featurep 'inf-clojure) 'inf-clojure)
>> +                                      ((featurep 'slime) 'slime)
>> +				      (t nil))
>
> What if users have, say, cider installed and also babashka executable?
> Will it be expected to use babashka?

Yes. The only thing that makes me slightly hesitant is that e.g.
someone doesn't have `bb` installed. Executes clojure source blocks
which are then evaluated in, let's say cider.
Then they install `bb` and the next time they start Emacs, the same
source block on re-evaluation would be executed with babashka.

I think this is still the best out of the box experience as it
"just works" for most users without having to customise something
and if they want it fixed, they can pin it to a certain backend.

What's your opinion?

Cheers,
  Daniel

Re: Auto detect ob-clojure backend (was: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list)

[Ihor Radchenko]

Daniel Kraus <daniel@kraus.my> writes:

>> What if users have, say, cider installed and also babashka executable?
>> Will it be expected to use babashka?
>
> Yes. The only thing that makes me slightly hesitant is that e.g.
> someone doesn't have `bb` installed. Executes clojure source blocks
> which are then evaluated in, let's say cider.
> Then they install `bb` and the next time they start Emacs, the same
> source block on re-evaluation would be executed with babashka.
>
> I think this is still the best out of the box experience as it
> "just works" for most users without having to customise something
> and if they want it fixed, they can pin it to a certain backend.
>
> What's your opinion?

I agree.

Though if you want to go really fancy, you can utilize org-persist to
store the customization value in persistent cache (org-persist-register),
load it back (org-persist-read), and re-use when the cached backend can
be enabled.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Re: Auto detect ob-clojure backend

[Bastien Guerry]

Ihor Radchenko <yantar92@posteo.net> writes:

>> What's your opinion?
>
> I agree.

+1 (FWIW)

-- 
 Bastien

Re: Auto detect ob-clojure backend (was: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list)

[Tim Cross]

Daniel Kraus <daniel@kraus.my> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> Daniel Kraus <daniel@kraus.my> writes:
>>
>>> +(defcustom org-babel-clojure-backend (cond
>>> +                                      ((executable-find "bb") 'babashka)
>>> +                                      ((executable-find "nbb") 'nbb)
>>> +                                      ((featurep 'cider) 'cider)
>>> +                                      ((featurep 'inf-clojure) 'inf-clojure)
>>> +                                      ((featurep 'slime) 'slime)
>>> +				      (t nil))
>>
>> What if users have, say, cider installed and also babashka executable?
>> Will it be expected to use babashka?
>
> Yes. The only thing that makes me slightly hesitant is that e.g.
> someone doesn't have `bb` installed. Executes clojure source blocks
> which are then evaluated in, let's say cider.
> Then they install `bb` and the next time they start Emacs, the same
> source block on re-evaluation would be executed with babashka.
>
> I think this is still the best out of the box experience as it
> "just works" for most users without having to customise something
> and if they want it fixed, they can pin it to a certain backend.
>
> What's your opinion?
>
> Cheers,
>   Daniel

Just chime in with my 2 cents.

I think bb is a much better solution from a babel perspective and would
love to see it as the default, even when you have both bb and cider
installed.

I stopped using clojure in org because it was way too fragile -
depending heavily on cider features which often changed. However, now we
have babashka and nbb, I'm thinking about using org again with clojure.

I recall looking at the babel code for clojure some time back to see if
it could be made simpler and more reliable. However, there wasn't much
that could be improved on given the design of cider and its focus on
interactive clojure development. I then thought using something like the
Clojure CLI tools might be the way to go. Now I feel that babashka for
clojure and nbb for clojurescript might be the right answer. 

Re: Auto detect ob-clojure backend (was: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list)

[Daniel Kraus]

Tim Cross <theophilusx@gmail.com> writes:

> I think bb is a much better solution from a babel perspective and would
> love to see it as the default, even when you have both bb and cider
> installed.

I just installed the patch. So if you have `bb` in your path, ob-clojure
should use babashka as default and only fall back to cider when it's not.

Currently it looks for bb, and then next `nbb` before cider.
Now I realise that nbb shouldn't even be in that list at all but instead
should be the default for `org-babel-execute:clojurescript`.

> I stopped using clojure in org because it was way too fragile -
> depending heavily on cider features which often changed. However, now we
> have babashka and nbb, I'm thinking about using org again with
> clojure.

Happy to hear feedback if you have any :)

> I recall looking at the babel code for clojure some time back to see if
> it could be made simpler and more reliable. However, there wasn't much
> that could be improved on given the design of cider and its focus on
> interactive clojure development.

I agree, cider is a pretty heavy library.
And I feel that if you have already "jacked in" in cider, you simply
eval straight from you clj(s) code instead of using org babel.
(Maybe in combination with clerk or something if you want it more literal)

> I then thought using something like the
> Clojure CLI tools might be the way to go.

I think I'll add a backend for the Clojure CLI tools.
Should be similar simple as bb and nbb with slower startup time,
but you would get a JVM Clojure for it.

> Now I feel that babashka for clojure and nbb for clojurescript
> might be the right answer.

Agree. bb default for Clojure and nbb for ClojureScript.

Thanks,
  Daniel

Re: Auto detect ob-clojure backend (was: [PATCH] Fix ob-clojure handling source block variable's value is a org-mode table or list)

[Christopher M. Miles]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]


Daniel Kraus <daniel@kraus.my> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> Daniel Kraus <daniel@kraus.my> writes:
>>
>>> +(defcustom org-babel-clojure-backend (cond
>>> +                                      ((executable-find "bb") 'babashka)
>>> +                                      ((executable-find "nbb") 'nbb)
>>> +                                      ((featurep 'cider) 'cider)
>>> +                                      ((featurep 'inf-clojure) 'inf-clojure)
>>> +                                      ((featurep 'slime) 'slime)
>>> +				      (t nil))
>>
>> What if users have, say, cider installed and also babashka executable?
>> Will it be expected to use babashka?
>
> Yes. The only thing that makes me slightly hesitant is that e.g.
> someone doesn't have `bb` installed. Executes clojure source blocks
> which are then evaluated in, let's say cider.
> Then they install `bb` and the next time they start Emacs, the same
> source block on re-evaluation would be executed with babashka.
>
> I think this is still the best out of the box experience as it
> "just works" for most users without having to customise something
> and if they want it fixed, they can pin it to a certain backend.
>
> What's your opinion?
>
> Cheers,
>   Daniel

I vote for use Clojure CLI like bb as default backend. Keeping defcustom
option for user to setting default backend is fine.

-- 

[ stardiviner ]
I try to make every word tell the meaning that I want to express without misunderstanding.

Blog: https://stardiviner.github.io/
IRC(libera.chat, freenode): stardiviner, Matrix: stardiviner
GPG: F09F650D7D674819892591401B5DF1C95AE89AC3

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]