From: Matt <matt@excalamus.com>
To: "Matt" <matt@excalamus.com>
Cc: "Ihor Radchenko" <yantar92@posteo.net>,
"emacs-orgmode" <emacs-orgmode@gnu.org>
Subject: bug? org-babel-comint-with-output return value type
Date: Sun, 17 Mar 2024 21:01:41 +0100 [thread overview]
Message-ID: <18e4e01717d.b1bd307a696132.1612686871442583784@excalamus.com> (raw)
In-Reply-To: <18e4deaa1bd.ff0f87fa694858.5955779335024266818@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
next prev parent reply other threads:[~2024-03-17 20:02 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-04 17:48 [BUG] Prompt appears in async shell results Matt
2024-02-05 14:02 ` Ihor Radchenko
2024-02-18 12:09 ` Matt
2024-02-19 11:10 ` Ihor Radchenko
2024-03-17 19:36 ` Matt
2024-03-17 20:01 ` Matt [this message]
2024-03-19 14:20 ` bug? org-babel-comint-with-output return value type Ihor Radchenko
2024-03-27 10:59 ` [BUG] Prompt appears in async shell results Ihor Radchenko
2024-03-29 11:15 ` Matt
2024-03-29 11:23 ` Ihor Radchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.orgmode.org/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=18e4e01717d.b1bd307a696132.1612686871442583784@excalamus.com \
--to=matt@excalamus.com \
--cc=emacs-orgmode@gnu.org \
--cc=yantar92@posteo.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).