bug? org-babel-comint-with-output return value type

[Matt <matt@excalamus.com>]

Looking at the last patch of https://list.orgmode.org/18e4deaa1bd.ff0f87fa694858.5955779335024266818@excalamus.com/, I would expect a reaction of something like, "WTF is all that weird org-trim string-join stuff on the prompt filter?"

---
 lisp/ob-comint.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index d13aacccc..f2251892a 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -325,9 +325,10 @@ STRING contains the output originally inserted into the comint buffer."
 		      until (and (equal (match-string 1) "start")
 				 (equal (match-string 2) uuid))
 		      finally return (+ 1 (match-end 0)))))
-		   ;; Apply callback to clean up the result
-		   (res-str (funcall org-babel-comint-async-chunk-callback
-                                     res-str-raw)))
+                   ;; Remove prompt
+                   (res-promptless (org-trim (string-join (mapcar #'org-trim (org-babel-comint--prompt-filter res-str-raw)) "\n") "\n"))
+		   ;; Apply user callback
+		   (res-str (funcall org-babel-comint-async-chunk-callback res-promptless)))
 	      ;; Search for uuid in associated org-buffers to insert results
 	      (cl-loop for buf in org-buffers
 		       until (with-current-buffer buf
-- 
2.41.0

The reason is that the current result of =org-babel-comint-with-output= is a list because the final expression is =delete=: https://git.savannah.gnu.org/cgit/emacs/org-mode.git/tree/lisp/ob-comint.el?id=46909a54e1a2ce0d948e94e0e19ff64af1a39eb9#n164

Why =org-babel-comint-with-output= returns a list is unclear to me.  The docstring for =org-babel-comint-with-output= says it should "return all process output".  AFAIK, process output is a string, always has been, always will be, and Babel has always gotten a substring of the process buffer (for that code path).  Yet for some reason, it was decided to have =org-babel-comint-with-output= return a list.  This goes back to the beginning, in 2009, when the final expression used =split-string= instead of =delete=: https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/lisp/org-babel-comint.el?id=dd0392a4f23b40fae7491c55aa44a2324248c103

You might ask, "But wouldn't returning a list require you to then convert it back into a string since =org-babel-comint-with-output= is really just a sentinel on process output?"  The answer is, "Yes, that's exactly what you'd need to do."

Call grep on =org-babel-comint-with-output= and you'll see in every case except the Ruby ':results value' results and Lua the ':results value' results, that indeed the return value of =org-babel-comint-with-output= is concatenated or the first string element taken.

| file          | line | result modified?         | result used? | comment                                                                      |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-ruby.el    |  263 | no                       | no           | used to send eoe-string                                                      |
|               |  272 | yes, mapconcat           | yes          | as returned value (for :results output)                                      |
|               |  281 | no                       | yes          | as returned value (for :results value)                                       |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-js.el      |  111 | yes, nth 1               | yes          | as returned value (for the default :results)                                 |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-shell.el   |  357 | yes, mapconcat           | yes          | as returned value (for session results of all types)                         |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-julia.el   |  327 | yes,  mapconcat          | yes          | as returned value (for :results output)                                      |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-lua.el     |  391 | yes, mapconcat           | yes          | as returned value (for :results output)                                      |
|               |  400 | no                       | yes          | as returned value (for :results value)                                       |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-haskell.el |  169 | yes, (cdr (reverse ...)) | yes          | as returned value? (for :results output) hard to tell, see lines 194 and 201 |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-octave.el  |  239 | yes, (cdr (reverse ...)) | yes          | as returned value (for :results output)                                      |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-ocaml.el   |   72 | yes, whoa...             | yes          | as returned value (for at least :results output)                             |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|
| ob-R          |  460 | yes, mapconcat           | yes          | as returned value (for :results output)                                      |
|---------------+------+--------------------------+--------------+------------------------------------------------------------------------------|

It looks like the original implementation aggregated process buffer output, collecting it into a list.  It then passed this list on to a function that did, as predicted, (cdr (reverse ...)) to get a string.  https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=dd0392a4f23b40fae7491c55aa44a2324248c103

It seems like the return type of =org-babel-comint-with-output= is not what it should be.  It looks like it should be a string.

Am I missing something?

Obviously, changing the return type would be a big job, requiring updates to at least 10 files, and has the potential to introduce bugs.  However, as far as I can tell, the return type isn't appropriate and so requires some kind of workaround, of which there are currently three.  I guess the best argument I can come up with at the moment for taking on the task of changing the return type is that, assuming my understanding is correct, the current implementation is simply an inappropriate, or outdated, design choice that only adds complexity.  My hope is that Babel continues to support new languages.  As that happens, they will need to apply work arounds, further entrenching the design and making it harder to correct.

What are people's thoughts on changing the return type of =org-babel-comint-with-output=?

--
Matt Trzcinski
Emacs Org contributor (ob-shell)
Learn more about Org mode at https://orgmode.org
Support Org development at https://liberapay.com/org-mode