Post-process table without changing result for empty table(/list)

Post-process table without changing result for empty table(/list)

[Jonas Bernoulli]

Hello,

If a code-block that is supposed to produce a table returns an empty
list, then nothing at all is inserted, as demonstrated by the second
example below:

#+begin_src emacs-lisp :results table
  ;; Some sql query that returns one or more rows:
  '(("foo" "bar"))
#+end_src
#+RESULTS:
| foo | bar |

#+begin_src emacs-lisp :results table
  ;; Another sql query that returns zero rows:
  '()
#+end_src
#+RESULTS:

I would like to keep it that way, but also use post-processing to add
a header to all non-empty tables (and only those).

Unfortunately post-processing changes how nil aka empty list aka empty
table is handled:

#+name: addheader
#+header: :var rows="" :var header=""
#+begin_src elisp :hlines yes
  (and rows `(("Key" "Value") hline ,@rows))
#+end_src

#+header: :post addheader(*this*,'("Key" "Value"))
#+begin_src emacs-lisp :results table
  ;; Some sql query that returns one or more rows:
  '(("foo" "bar"))
#+end_src
#+RESULTS:
| Key | Value |
|-----+-------|
| foo | bar   |

#+header: :post addheader(*this*,'("Key" "Value"))
#+begin_src emacs-lisp :results table
  ;; Another sql query that returns zero rows:
  '()
#+end_src
#+RESULTS:
: nil

I would like it to result in just

#+RESULTS:

like when no post-processing is involved.

It used to behave like that before 51a628bc5efc from 2009, which started
turning all symbols, including nil, into strings, but without giving any
reason why that should be done.

It has worked like this for a long time now, so reverting that is
probably not feasible in the short run.  However, I feel it would
make sense to change now how nil/'() is treated.  Currently it is
being treated as the symbol nil, but IMO it would make more sense
to treat it as the empty list.  That could be achieved with

diff --git a/lisp/ob-ref.el b/lisp/ob-ref.el
index b79e47900..2b4a16aea 100644
--- a/lisp/ob-ref.el
+++ b/lisp/ob-ref.el
@@ -199,7 +199,7 @@ (defun org-babel-ref-resolve (ref)
                                (org-babel-execute-src-block nil info params))))
                     (error "Reference `%s' not found in this buffer" ref))))
             (cond
-             ((symbolp result) (format "%S" result))
+             ((and result (symbolp result)) (format "%S" result))
              ((and index (listp result))
               (org-babel-ref-index-list index result))
              (t result)))))))))

In the long run you might want to consider not turning any symbols
into strings, at least not when the "regular" block as well as the
post-processing block both use elisp.

     Thanks for considering my suggestions!
     Jonas

Re: Post-process table without changing result for empty table(/list)

[Ihor Radchenko]

Jonas Bernoulli <jonas@bernoul.li> writes:

> It used to behave like that before 51a628bc5efc from 2009, which started
> turning all symbols, including nil, into strings, but without giving any
> reason why that should be done.
>
> It has worked like this for a long time now, so reverting that is
> probably not feasible in the short run.  However, I feel it would
> make sense to change now how nil/'() is treated.  Currently it is
> being treated as the symbol nil, but IMO it would make more sense
> to treat it as the empty list.  That could be achieved with
>
> diff --git a/lisp/ob-ref.el b/lisp/ob-ref.el
> index b79e47900..2b4a16aea 100644
> --- a/lisp/ob-ref.el
> +++ b/lisp/ob-ref.el
> @@ -199,7 +199,7 @@ (defun org-babel-ref-resolve (ref)
>                                 (org-babel-execute-src-block nil info params))))
>                      (error "Reference `%s' not found in this buffer" ref))))
>              (cond
> -             ((symbolp result) (format "%S" result))
> +             ((and result (symbolp result)) (format "%S" result))
>               ((and index (listp result))
>                (org-babel-ref-index-list index result))
>               (t result)))))))))

Looks reasonable.
Could you please prepare a patch and possibly also add a test that
covers your use-case to testing/lisp/test-ob.el?
See https://orgmode.org/worg/org-contribute.html

> In the long run you might want to consider not turning any symbols
> into strings, at least not when the "regular" block as well as the
> post-processing block both use elisp.

This may be tricky. Introducing any kind of special case will make the
code fragile. We should better make things as generic as possible.

-- 
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: Post-process table without changing result for empty table(/list)

[Jonas Bernoulli]

Ihor Radchenko <yantar92@gmail.com> writes:

> Jonas Bernoulli <jonas@bernoul.li> writes:
>
>> It used to behave like that before 51a628bc5efc from 2009, which started
>> turning all symbols, including nil, into strings, but without giving any
>> reason why that should be done.
>>
>> It has worked like this for a long time now, so reverting that is
>> probably not feasible in the short run.  However, I feel it would
>> make sense to change now how nil/'() is treated.  Currently it is
>> being treated as the symbol nil, but IMO it would make more sense
>> to treat it as the empty list.  That could be achieved with
>>
>> diff --git a/lisp/ob-ref.el b/lisp/ob-ref.el
>> index b79e47900..2b4a16aea 100644
>> --- a/lisp/ob-ref.el
>> +++ b/lisp/ob-ref.el
>> @@ -199,7 +199,7 @@ (defun org-babel-ref-resolve (ref)
>>                                 (org-babel-execute-src-block nil info params))))
>>                      (error "Reference `%s' not found in this buffer" ref))))
>>              (cond
>> -             ((symbolp result) (format "%S" result))
>> +             ((and result (symbolp result)) (format "%S" result))
>>               ((and index (listp result))
>>                (org-babel-ref-index-list index result))
>>               (t result)))))))))
>
> Looks reasonable.
> Could you please prepare a patch and possibly also add a test that
> covers your use-case to testing/lisp/test-ob.el?
> See https://orgmode.org/worg/org-contribute.html

Will do.

>> In the long run you might want to consider not turning any symbols
>> into strings, at least not when the "regular" block as well as the
>> post-processing block both use elisp.
>
> This may be tricky. Introducing any kind of special case will make the
> code fragile. We should better make things as generic as possible.

By "special case", do you mean "just for elisp", or "if both use the
same language, whatever that might be"?  IMO it would be best if as much
information were preserved between the two code blocks, and when they
use the same language, that should be "all of it", or nearly.

If they use the same language that might be fairly easy to do (bypass
the code that previously prevented it), but of course it would be
preferable if all type information were preserved between any two block.

But that would be harder, which is why I would suggest to first/only do
it if the same language is used by both blocks.  The actual suggestion
to do it only if both block use elisp, was more about first trying it
with the language we are most familiar with.  I wasn't trying to imply
it should only be done for that language.  Of course, if initially only
doing for elisp actually makes it harder, then doing it for all
languages at once, would be preferable.

---

Speaking of other languages, when I investigated the above issue, I
tried whether the issue was maybe limited to post-processing blocks that
use elisp.  So I also tried doing it using python, but it turned out
that that had the same issue, and additionally there was a somewhat
related, python specific, bug:

`org-babel-script-escape' doesn't handle an empty python list correctly:
  "['a']" => ("a")
but
  "[]"    => []
I.e., an empty python list is turned into an empty lisp vector instead
of an empty lisp list.  At least for python, (> (length str) 2) should
probably be changed to use >=.

---

And while reproducing that issue just now, I ran into an additional,
unrelated issue.  I didn't have python installed and when I tried to
evaluate a python block directly, that resulted in an error as expected.
However, when I evaluated a elisp block, which uses a post-processing
block that uses python, that failed silently.

Re: Post-process table without changing result for empty table(/list)

[Ihor Radchenko]

Jonas Bernoulli <jonas@bernoul.li> writes:

>>> In the long run you might want to consider not turning any symbols
>>> into strings, at least not when the "regular" block as well as the
>>> post-processing block both use elisp.
>>
>> This may be tricky. Introducing any kind of special case will make the
>> code fragile. We should better make things as generic as possible.
>
> By "special case", do you mean "just for elisp", or "if both use the
> same language, whatever that might be"?  IMO it would be best if as much
> information were preserved between the two code blocks, and when they
> use the same language, that should be "all of it", or nearly.

Either way. It's not like I am against the idea. Rather (1) afraid to
over-complicate the already complex babel code; (2) afraid to break
something non-trivial.

If we do it, I'd prefer same language / same language. Basically, as
generic as possible approach that will be easier to maintain.

> If they use the same language that might be fairly easy to do (bypass
> the code that previously prevented it), but of course it would be
> preferable if all type information were preserved between any two block.
>
> But that would be harder, which is why I would suggest to first/only do
> it if the same language is used by both blocks.  The actual suggestion
> to do it only if both block use elisp, was more about first trying it
> with the language we are most familiar with.  I wasn't trying to imply
> it should only be done for that language.  Of course, if initially only
> doing for elisp actually makes it harder, then doing it for all
> languages at once, would be preferable.

I do no like to make things elisp-specific, unless you can fit it into
ob-emacs-lisp.el. For more generic things, we also need to be careful
and not break anything.

> Speaking of other languages, when I investigated the above issue, I
> tried whether the issue was maybe limited to post-processing blocks that
> use elisp.  So I also tried doing it using python, but it turned out
> that that had the same issue, and additionally there was a somewhat
> related, python specific, bug:
>
> `org-babel-script-escape' doesn't handle an empty python list correctly:
>   "['a']" => ("a")
> but
>   "[]"    => []
> I.e., an empty python list is turned into an empty lisp vector instead
> of an empty lisp list.  At least for python, (> (length str) 2) should
> probably be changed to use >=.

Patches welcome. But please change the subject in the email containing
the patch for easier tracking.

> ---
>
> And while reproducing that issue just now, I ran into an additional,
> unrelated issue.  I didn't have python installed and when I tried to
> evaluate a python block directly, that resulted in an error as expected.
> However, when I evaluated a elisp block, which uses a post-processing
> block that uses python, that failed silently.

It would help if you can provide a reproducer. Also in a branched
thread.

-- 
Ihor Radchenko,
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

[PATCH 0/1] Allow returning empty list from post-processing block

[Jonas Bernoulli]

Here's my patch, including a test and a news entry.

I didn't know where to put the test so I just put it at the beginning.
It should probably be moved.

Jonas Bernoulli (1):
  Allow returning empty list from post-processing block

 etc/ORG-NEWS            |  6 ++++++
 lisp/ob-ref.el          |  2 +-
 testing/lisp/test-ob.el | 24 ++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)

-- 
2.37.3

[PATCH 1/1] Allow returning empty list from post-processing block

[Jonas Bernoulli]

* lisp/ob-ref.el (org-babel-ref-resolve): When the result an empty
list, then treat it as a list, not as the symbol nil.
* testing/lisp/test-ob.el (test-ob/return-empty-list-from-post-block):
Add new test.
* etc/ORG-NEWS (Post-processing code blocks can return an empty list):
Document change in behavior.
---
 etc/ORG-NEWS            |  6 ++++++
 lisp/ob-ref.el          |  2 +-
 testing/lisp/test-ob.el | 24 ++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 4728528f8..74095d101 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -425,6 +425,12 @@ The new variable name is =org-plantuml-args=.  It now applies to both
 jar PlantUML file and executable.
 
 ** Miscellaneous
+*** Post-processing code blocks can return an empty list
+
+When the result of a regular code block is nil, then that was already
+treated as an empty list.  Now that is also the case for code blocks
+that post-process the result of another block.
+
 *** Styles are customizable in ~biblatex~ citation processor
 
 It is now possible to add new styles or modify old ones in ~biblatex~
diff --git a/lisp/ob-ref.el b/lisp/ob-ref.el
index b79e47900..2b4a16aea 100644
--- a/lisp/ob-ref.el
+++ b/lisp/ob-ref.el
@@ -199,7 +199,7 @@ (defun org-babel-ref-resolve (ref)
 				(org-babel-execute-src-block nil info params))))
 		     (error "Reference `%s' not found in this buffer" ref))))
 	     (cond
-	      ((symbolp result) (format "%S" result))
+	      ((and result (symbolp result)) (format "%S" result))
 	      ((and index (listp result))
 	       (org-babel-ref-index-list index result))
 	      (t result)))))))))
diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el
index 99912fe63..8d1548fe1 100644
--- a/testing/lisp/test-ob.el
+++ b/testing/lisp/test-ob.el
@@ -26,6 +26,30 @@ (require 'ob-ref)
 (require 'org-table)
 (eval-and-compile (require 'cl-lib))
 
+(ert-deftest test-ob/return-empty-list-from-post-block ()
+  "When the result of a post-processing source block is an empty
+list, then it should be treated as such; not as the symbol nil."
+  (should
+   (let ((default-directory temporary-file-directory))
+     (org-test-with-temp-text
+         "
+#+name: addheader
+#+header: :var rows=\"\"
+#+begin_src elisp :hlines yes
+  '()
+#+end_src
+#+header: :post addheader(*this*)
+#+<point>begin_src emacs-lisp :results table
+#+end_src
+#+RESULTS:
+: nil"
+       (org-babel-execute-src-block)
+       (goto-char (1- (point-max)))
+       (equal (buffer-substring-no-properties
+               (line-beginning-position)
+               (line-end-position))
+              "#+RESULTS:")))))
+
 (ert-deftest test-ob/indented-cached-org-bracket-link ()
   "When the result of a source block is a cached indented link it
 should still return the link."
-- 
2.37.3

Re: [PATCH 1/1] Allow returning empty list from post-processing block

[Ihor Radchenko]

Jonas Bernoulli <jonas@bernoul.li> writes:

> * lisp/ob-ref.el (org-babel-ref-resolve): When the result an empty
> list, then treat it as a list, not as the symbol nil.
> * testing/lisp/test-ob.el (test-ob/return-empty-list-from-post-block):
> Add new test.
> * etc/ORG-NEWS (Post-processing code blocks can return an empty list):
> Document change in behavior.

Thanks for the patch!
LGTM in general.

>  
> +(ert-deftest test-ob/return-empty-list-from-post-block ()

Re: test location. Should probably be around
`test-ob/elisp-in-header-arguments'.

Also, I'd slightly prefer to have a more general test name like
test-ob/post-header-argument. We may add more cases inside later.

I have no comments wrt the code in the commit.

-- 
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>

[PATCH v1] Allow returning empty list from post-processing block

[Jonas Bernoulli]

* lisp/ob-ref.el (org-babel-ref-resolve): When the result an empty
list, then treat it as a list, not as the symbol nil.
* testing/lisp/test-ob.el (test-ob/post-header-arguments): Add new
test.
* etc/ORG-NEWS (Post-processing code blocks can return an empty list):
Document change in behavior.
---
 etc/ORG-NEWS            |  6 ++++++
 lisp/ob-ref.el          |  2 +-
 testing/lisp/test-ob.el | 24 ++++++++++++++++++++++++
 3 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 4728528f8..74095d101 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -425,6 +425,12 @@ The new variable name is =org-plantuml-args=.  It now applies to both
 jar PlantUML file and executable.
 
 ** Miscellaneous
+*** Post-processing code blocks can return an empty list
+
+When the result of a regular code block is nil, then that was already
+treated as an empty list.  Now that is also the case for code blocks
+that post-process the result of another block.
+
 *** Styles are customizable in ~biblatex~ citation processor
 
 It is now possible to add new styles or modify old ones in ~biblatex~
diff --git a/lisp/ob-ref.el b/lisp/ob-ref.el
index b79e47900..2b4a16aea 100644
--- a/lisp/ob-ref.el
+++ b/lisp/ob-ref.el
@@ -199,7 +199,7 @@ (defun org-babel-ref-resolve (ref)
 				(org-babel-execute-src-block nil info params))))
 		     (error "Reference `%s' not found in this buffer" ref))))
 	     (cond
-	      ((symbolp result) (format "%S" result))
+	      ((and result (symbolp result)) (format "%S" result))
 	      ((and index (listp result))
 	       (org-babel-ref-index-list index result))
 	      (t result)))))))))
diff --git a/testing/lisp/test-ob.el b/testing/lisp/test-ob.el
index 99912fe63..9051a642d 100644
--- a/testing/lisp/test-ob.el
+++ b/testing/lisp/test-ob.el
@@ -136,6 +136,30 @@ (ert-deftest test-ob/get-src-block-info-tangle ()
     (let ((info (org-babel-get-src-block-info)))
       (should (string= "no" (cdr (assq :tangle (nth 2 info))))))))
 
+(ert-deftest test-ob/post-header-arguments ()
+  "When the result of a post-processing source block is an empty
+list, then it should be treated as such; not as the symbol nil."
+  (should
+   (let ((default-directory temporary-file-directory))
+     (org-test-with-temp-text
+         "
+#+name: addheader
+#+header: :var rows=\"\"
+#+begin_src elisp :hlines yes
+  '()
+#+end_src
+#+header: :post addheader(*this*)
+#+<point>begin_src emacs-lisp :results table
+#+end_src
+#+RESULTS:
+: nil"
+       (org-babel-execute-src-block)
+       (goto-char (1- (point-max)))
+       (equal (buffer-substring-no-properties
+               (line-beginning-position)
+               (line-end-position))
+              "#+RESULTS:")))))
+
 (ert-deftest test-ob/elisp-in-header-arguments ()
   "Test execution of elisp forms in header arguments."
   (org-test-with-temp-text-in-file "
-- 
2.37.3

Re: [PATCH v1] Allow returning empty list from post-processing block

[Ihor Radchenko]

Jonas Bernoulli <jonas@bernoul.li> writes:

> * lisp/ob-ref.el (org-babel-ref-resolve): When the result an empty
> list, then treat it as a list, not as the symbol nil.
> * testing/lisp/test-ob.el (test-ob/post-header-arguments): Add new
> test.
> * etc/ORG-NEWS (Post-processing code blocks can return an empty list):
> Document change in behavior.

Applied onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=9b690462a319e4268f15949bba0fe024dca77182

Thanks!

-- 
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>