Hi!

Ihor Radchenko <yantar92@posteo.net> writes:

> You need to document the changes in ORG-NEWS.

I added an entry about the new ClojureScript change.
I guess the other (main) part of the patch is a bugfix and will not
be documented in ORG-NEWS.

> Also, see inline comments below.
>
>> Subject: [PATCH] ob-clojure.el: Fix results output, support clojure-cli
>
> Does it have to be a single commit? Not a big deal, but two separate
> commits would be cleaner.

I saw that as well but unfortunately I did it all in the same go
and then touch the same code parts, so separating them in individual
working commits, while not a huge task, is still some work with not
much benefit.

>> -;; Support for evaluating Clojure code
>> +;; Support for evaluating Clojure / ClojureScript code.
>
> This is an important new feature that should be announced clearly in ORG-NEWS.

ClojureScript source blocks where supported before, but just no way to set
the backend for it separately. See the ORG-NEWS in the new attached patch.

> Also, make sure that
> https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-clojure.html
> is up-to-date.

Good reminder. I send a PR for this when this patch is installed?!


>>  (defcustom org-babel-clojure-backend (cond
>>                                        ((executable-find "bb") 'babashka)
>> -                                      ((executable-find "nbb") 'nbb)
>> +                                      ((executable-find "clojure") 'clojure-cli)
>>                                        ((featurep 'cider) 'cider)
>>                                        ((featurep 'inf-clojure) 'inf-clojure)
>>                                        ((featurep 'slime) 'slime)
>> @@ -87,11 +88,24 @@ defcustom org-babel-clojure-backend
>>    :group 'org-babel
>>    :package-version '(Org . "9.6")
>
> Org 9.7. This is a new feature to come in the next release.

Fixed.

>> +(defcustom org-babel-clojurescript-backend
>> +  (cond
>> +   ((or (executable-find "nbb") (executable-find "npx")) 'nbb)
>> +   ((featurep 'cider) 'cider)
>> +   (t nil))
>> +  "Backend used to evaluate Clojure code blocks."
>> +  :group 'org-babel
>> +  :package-version '(Org . "9.6")
>
> 9.7
> New customization to be announced in ORG-NEWS.

Fixed.

>> -(defcustom ob-clojure-nbb-command (executable-find "nbb")
>> +(defcustom ob-clojure-nbb-command (or (executable-find "nbb")
>> +                                      (when-let (npx (executable-find "npx"))
>> +                                        (concat npx " nbb")))
>>    "Path to the nbb executable."
>>    :type '(choice file (const nil))
>> +  :group 'org-babel)
>
> It is no longer a path, despite what is claimed in the docstring.
> :type definition is also not accurate.
> Also, update :package-version.

Fixed.

>> +(defcustom ob-clojure-cli-command (when-let (cmd (executable-find "clojure"))
>> +                                    (concat cmd " -M"))
>> +  "Clojure CLI command used to execute source blocks."
>> +  :type 'file
>>    :group 'org-babel
>>    :package-version '(Org . "9.6"))
>
> 9.7

Fixed.

>>  (defun org-babel-expand-body:clojure (body params)
>>    "Expand BODY according to PARAMS, return the expanded body."
>>    (let* ((vars (org-babel--get-vars params))
>> +         (cljs-p (string= (cdr (assq :target params)) "cljs"))
>
> Note: I do not see :target header arg being documented in
> https://orgmode.org/worg/org-contrib/babel/languages/ob-doc-clojure.html

This was apparently a kludge that ob-clojure used to evaluate ClojureScript
in the normal clojure:execute function.
I simply used the same kludge where I need to check for cljs, but after
reviewing it's not really necessary and I removed the :target parameter
completely. As this was undocumented I guess it's ok to remove?!

>> -(defun ob-clojure-eval-with-babashka (bb expanded)
>> -  "Evaluate EXPANDED code block using BB (babashka or nbb)."
>> -  (let ((script-file (org-babel-temp-file "clojure-bb-script-" ".clj")))
>
> This will remove a non-private function. May you leave a fallback
> obsolete alias to not break third-party code that calls the old function
> name?

I created an obsolete-function-alias.


Attached is the new patch with the changes.

Thanks,
  Daniel