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

[Richard Lawrence <rwl@recursewithless.net>]

Ihor Radchenko <yantar92@posteo.net> writes:
         
>> 2) I'm finding org-element-create rather unintuitive to work with, I
>>    think because of its &rest argument for children and the handling of
>>    "anonymous" nodes.
>> ...
>>    As it is, I can't figure out how to call org-element-create and
>>    org-element-contents in the right way to set the contents of a node
>>    to a list of other nodes, and return that list from
>>    org-element-contents.
>
> Should be just
> (org-element-create type props contents)
> and then
> (org-element-contents new-node)

That's what I would have thought. In my experiment, I have done this:

(defun ical:make-ast-node (type &optional props &rest children)
  ;; automatically mark :value as a seconary property for org-element-ast:
  (let ((full-props (plist-put props :secondary (list :value))))
        (org-element-create type full-props children)))

(so basically just aliasing org-element-create) and

(defalias 'ical:ast-node-children #'org-element-contents)

I also changed all my calls to ical:make-ast-node to reflect the calling
convention of org-element-create, e.g. in ical:read-property-value I now
have

(ical:make-ast-node type
                    (list :value value :original-value unrecognized-val)
                    params)

The calls to ical:ast-node-children shouldn't need adjusting, since both
the old version and org-element-set-contents just take a single
argument, the syntax node.

But when I run my parser tests with the above changes, they (almost) all
fail with errors like this:

Test string:
"ATTENDEE;RSVP=TRUE;ROLE=REQ-PARTICIPANT:mailto:jsmith@example.com\n"

Error:
    (icalendar-validation-error
     "`icalendar-attendee' may not contain `nil'"
     (icalendar-attendee
      (:standard-properties
       [1 nil nil nil 66 nil (:value) nil nil nil ...] :value
       (icalendar-cal-address
        (:standard-properties
         [nil nil nil nil nil nil ... nil nil nil ...] :value
         "mailto:jsmith@example.com")
        nil)
       :original-value nil :value-begin 41 :value-end 66)
      ((icalendar-rsvpparam
        (:standard-properties [10 nil nil nil 19 nil ... nil nil nil ...]
                              :value (icalendar-boolean ... nil)
                              :original-value nil :value-begin 15
                              :value-end 19)
        nil)
       (icalendar-roleparam
        (:standard-properties [20 nil nil nil 40 nil ... nil nil nil ...]
                              :value "REQ-PARTICIPANT" :original-value nil
                              :value-begin 25 :value-end 40)
        nil))))

And probing with the debugger reveals e.g. that (icalendar-node-children node)
returns in this example:

(((icalendar-rsvpparam
   (:standard-properties
    [10 nil nil nil 19 nil (:value) nil nil nil nil nil nil nil
        #<buffer *iCalendar Parse Test*> nil nil nil]
    :value
    (icalendar-boolean
     (:standard-properties
      [nil nil nil nil nil nil (:value) nil nil nil nil nil nil nil
           nil nil nil nil]
      :value t)
     nil)
    :original-value nil :value-begin 15 :value-end 19)
   nil)
  (icalendar-roleparam
   (:standard-properties
    [20 nil nil nil 40 nil (:value) nil nil nil nil nil nil nil
        #<buffer *iCalendar Parse Test*> nil nil nil]
    :value "REQ-PARTICIPANT" :original-value nil :value-begin 25
    :value-end 40)
   nil)))

That is, the list of parameter nodes is wrapped in an extra list, which
causes my function which counts children by type to fail and signal the
error.

Initially I thought that this is because org-element-contents is
implemented in the relevant case as (nthcdr 2 node) rather than e.g.
(nth 2 node). Changing the definition of ical:ast-node-children from the
above alias to

(defun ical:ast-node-children (node)
  (nth 2 node))

helps -- more tests pass -- but I'm not sure why this change should be
necessary. (I feel like I'm missing something really obvious here but so
far it eludes me!)

Even with this change, most tests still fail with an error like the
above, because an *empty* list of children is still returned as (nil)
rather than nil. The problem here is apparently that I am passing nil as
an explicit &rest argument to org-element-create; if I then change the
definition of ical:make-ast-node to work around this, e.g.

(defun ical:make-ast-node (type &optional props &rest children)
  ;; automatically mark :value as a seconary property for org-element-ast:
  (let ((full-props (plist-put props :secondary (list :value))))
    (if (equal children '(nil))
        (org-element-create type full-props)
      (org-element-create type full-props children))))

then all the tests pass again. But this is where I find the calling
convention of org-element-create unintuitive and would prefer an
interface where I can just always pass a list of child nodes, including
an empty list, and org-element-contents will always give me exactly that
list back.

This got a bit long, but I hope it's detailed enough to make things clear!

Best,
Richard