Re: Upstreaming org-element-ast (was: Improving Emacs' iCalendar support)

[Ihor Radchenko]

Richard Lawrence <rwl@recursewithless.net> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> I think I can just add a special case to `org-element-create':
>> when CHILDREN is (nil), ignore it.
>>
>> Does it make sense?
>
> That would be enough for my purposes. I don't know if it's enough for
> Org's (and worry that the change might introduce hard-to-find bugs). I
> wonder therefore if it might be better to create a wrapper with a
> different argument list like ...

I think that yet another function will simply add to the confusion.
As for bugs, org-element-ast has tests that should prevent such issues.

Attaching tentative patch that will make `org-element-create' ignore
nils in CHILDREN. nils are not valid nodes anyway.

>From 59d425a1af9829825d771ead63f73efe614044ba Mon Sep 17 00:00:00 2001
Message-ID: 
<59d425a1af9829825d771ead63f73efe614044ba.1736672541.git.yantar92@posteo.net>
From: Ihor Radchenko <yantar92@posteo.net>
Date: Sun, 12 Jan 2025 10:01:24 +0100
Subject: [PATCH] org-element-create: Ignore nils in CHILDREN

* lisp/org-element-ast.el (org-element-create): When CHILDREN contain
nil entries, ignore them.
* testing/lisp/test-org-element.el (test-org-element/org-element-create):
Add more test cases.
* etc/ORG-NEWS (~org-element-create~ now ignores ~nil~s in CHILDREN argument):
Announce the change.

Link: 87sepr2ee5.fsf@recursewithless.net">https://yhetil.org/emacs-devel/87sepr2ee5.fsf@recursewithless.net
---
 etc/ORG-NEWS                     | 11 +++++++++++
 lisp/org-element-ast.el          | 16 ++++++++++++----
 testing/lisp/test-org-element.el | 10 +++++++++-
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index aebd0dedd7..adc48f3044 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -365,6 +365,17 @@ elisp level ~org-datetree-find-create-entry~ and
 ~org-datetree-find-date-create~, ~org-datetree-find-month-create~, and
 ~org-datetree-find-iso-week-create~ to new datetree types.
 
+*** ~org-element-create~ now ignores ~nil~s in CHILDREN argument
+
+When CHILDREN contains ~nil~ elements, they are skipped.  This way,
+
+#+begin_src emacs-lisp
+(let ((children nil))
+  (org-element-create 'section nil children)) ; => (section nil)
+#+end_src
+
+will yield expected results rather than assigning literal ~nil~ as a child.
+
 ** Removed or renamed functions and variables
 
 *** ~org-cycle-display-inline-images~ is renamed to 
~org-cycle-display-link-previews~
diff --git a/lisp/org-element-ast.el b/lisp/org-element-ast.el
index 69a37c26a1..f56a76a28a 100644
--- a/lisp/org-element-ast.el
+++ b/lisp/org-element-ast.el
@@ -729,6 +729,11 @@ (defun org-element-create (type &optional props &rest 
children)
 will yield expected results with contents of another node adopted into
 a newly created one.
 
+nil elements in CHILDREN are ignored.  This way,
+   (let ((children nil))
+     (org-element-create \\='section nil children))
+will yield expected results.
+
 When TYPE is `plain-text', CHILDREN must contain a single node -
 string.  Alternatively, TYPE can be a string.  When TYPE is nil or
 `anonymous', PROPS must be nil."
@@ -736,6 +741,12 @@ (defun org-element-create (type &optional props &rest 
children)
    ;; FIXME: Just use `plistp' from Emacs 29 when available.
    (let ((len (proper-list-p props)))
      (and len (zerop (% len 2)))))
+  ;; Special case: CHILDREN is a single anonymous node
+  (when (and (= 1 (length children))
+             (org-element-type-p (car children) 'anonymous))
+    (setq children (car children)))
+  ;; Filter out nil values from CHILDREN
+  (setq children (delq nil children))
   ;; Assign parray.
   (when (and props (not (stringp type)) (not (eq type 'plain-text)))
     (let ((node (list 'dummy props)))
@@ -762,10 +773,7 @@ (defun org-element-create (type &optional props &rest 
children)
     ((pred stringp)
      (if props (org-add-props type props) type))
     (_
-     (if (and (= 1 (length children))
-              (org-element-type-p (car children) 'anonymous))
-         (apply #'org-element-adopt (list type props) (car children))
-       (apply #'org-element-adopt (list type props) children)))))
+     (apply #'org-element-adopt (list type props) children))))
 
 (defun org-element-copy (datum &optional keep-contents)
   "Return a copy of DATUM.
diff --git a/testing/lisp/test-org-element.el b/testing/lisp/test-org-element.el
index ad96c3106d..d4ec00fba3 100644
--- a/testing/lisp/test-org-element.el
+++ b/testing/lisp/test-org-element.el
@@ -515,7 +515,15 @@ (ert-deftest test-org-element/org-element-create ()
   ;; Children
   (let ((children '("a" "b" (org-element-create 'foo))))
     (should (equal (cddr (apply #'org-element-create 'bar nil children))
-                   children))))
+                   children)))
+  (let ((children nil))
+    (should (equal (cddr (org-element-create 'bar nil children))
+                   children))
+    (should (equal (cddr (apply #'org-element-create 'bar nil children))
+                   children)))
+  (let ((children '("foo" nil "bar")))
+    (should (equal (cddr (apply #'org-element-create 'bar nil children))
+                   (delq nil children)))))
 
 (ert-deftest test-org-element/put-property ()
   "Test `org-element-put-property' specifications."
-- 
2.47.1

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
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>