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

[Matt]

Looking at the last patch of 
18e4deaa1bd.ff0f87fa694858.5955779335024266818@excalamus.com/">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