Subject: bug#74994: Improve Emacs iCalendar support

From: Stefan Monnier <[email-masked]>
Date: Tue, 21 Jan 2025 18:33:02 -0500

>> I've been thinking it would make sense eventually to:
>>   - rename icalendar.el to something like diary-icalendar.el
>>   - rewrite the code there to use the new parser in icalendar-parser.el

I don't have an opinion on the file naming part, but I suggest you scrap
the "eventually" above, otherwise there's a strong risk it'll never
happen (either for lack of interest or because the API that doesn't
quite fit).

>> 2) It would be nice if someone who has more experience writing parsers
>> could give me some feedback about the overall design of the parser. In
>> particular:

[ Note: I have not yet had time to look at the code.  ]

>>   - I've used macros (see icalendar-macs.el) to create a small "DSL" for
>>     defining iCalendar types. These macros store parsing-related information for
>>     each type as properties of the symbols which name them. There's a lot of
>>     dynamic dispatch in the parser based on these type symbols' properties.
>>     This adds some complexity but (I hope) makes the parser more "atomic"/
>>     extensible. Does this seem like a reasonable approach in general?

It sounds like a reasonable design, yes.

In `bindat.el` I used a similar approach except that each construct (I
guess in your case, that means each "type") is stored as a method (in
a generic function) instead of a property of a symbol.  I'm not sure
it's the perfect solution, but it's nice that `C-h o` on the generic
function can then provide a documentation of each of the constructs.

Other options we use elsewhere is to use function names constructed from
a constant prefix plus the name of the construct, so instead of

    (funcall (get 'foo 'bar) ...)

you might be able to macroexpand to something like

    (,(intern (format "bar %s" 'foo)) ...)

so you get (for free) compile-time warnings when using a construct that
doesn't exist, and you avoid a `get` at runtime (IIRC, we use that
approach in `peg.el`).

>>   - I ran into one issue that feels like a design flaw: the parser separates
>>     "reading" (converting a string to an Elisp value) into a function
>>     distinct from the parsing function which matches that string (see e.g.
>>     ical:parse-property-value in icalendar-parser.el, which calls
>>     ical:read-property-value). In simple cases this nicely factors out a pure
>>     function from one which depends on a lot of global buffer state;
>>     but in more complicated cases the "pure" reader function depends on
>>     the match data and so isn't pure at all (see e.g. ical:read-dur-value).
>>     Is there a better way to do this? (Not make the distinction? Pass
>>     the match data explicitly? ...?)

Is the separation useful to users (including internal users) of the
parser?  This kind of problem doesn't directly ring a bell, so I don't
have a good suggestion to make.  Maybe when I get time to read the
code...  In the mean time, I can just mention that it reminds of the PEG
parsers generated by `peg.el` which use a limited form of backtracking:
they have a pure(ish) part that "explores" and accumulates "actions to
perform if we find a match" and only once we found a global parse do we
actually run the (side-effecting) actions which might build a parse tree.

>>   - whether an icalendar-mode is even a useful thing to have, and what
>>     could make it more useful

I don't think we need to be shy about major modes.  If you don't use it,
it doesn't cost much (because it's not loaded), so count me as "+1".

>>   - whether the approach to syntax highlighting is reasonable (the
>>     icalendar-define-* macros define matching functions for each
>>     individual icalendar type, which are eventually piled
>>     into a long list of matchers in the mode's value for font-lock-keywords)
>>   - what still needs to be done to make the code in icalendar-mode.el
>>     compliant with Emacs' major mode conventions (see the long list of
>>     TODOs in the mode definition)

[ Will see when I finally read the code.  ]

>>   - whether there's a better solution to the problem of needing to unfold
>>     lines *before* a buffer containing iCalendar data is decoded
>>     (is there anything like a hook that runs before decoding?)

[ Sorry, I don't understand this question.  ]

        Stefan