GNU bug report logs - #74994
Improve Emacs iCalendar support

Previous Next

Full log

Message #44 received at 74994 <at> debbugs.gnu.org (full text, mbox):

From: Richard Lawrence <rwl <at> recursewithless.net>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 74994 <at> debbugs.gnu.org
Subject: Re: bug#74994: Acknowledgement (Improve Emacs iCalendar support)
Date: Sun, 19 Jan 2025 21:24:04 +0100

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Richard Lawrence <rwl <at> recursewithless.net>
>> Date: Sun, 19 Jan 2025 08:59:30 +0100
>> 
>> I know this bug was opened right before the holidays, and it's a big
>> patch, so I understand if no one's had time to look at it yet. Before I
>> put any more work into this, though, it would useful if someone could
>> just give this a quick look and tell me if it seems like I'm headed in
>> the right direction: a "yes, this looks reasonable, let us know when
>> you're finished" or a "there's no way this will get merged, please start
>> over with a new approach" would be helpful feedback to have at this
>> point. (Of course, more detailed feedback is welcome too, if someone has
>> time.)
>
> You posted 2 patches, both of them quite large.  We don't have
> icalendar experts on board whom we could ask to review the patches.

Right, I guess at this point that "expert" may be me. :)

I wasn't looking for any very detailed feedback, especially about
anything iCalendar-specific, just a quick check-in to make sure I
haven't already made some decision that would prevent the code from
being merged, out of my own inexperience.

> Are there any specific aspects of the patches you'd like us to look
> into, or some specific design issues you'd like to discuss?  That'd
> make the review more focused.

Here are a few things I've been wondering about, for anyone who has time
to take a look:

1) Is it alright that this is a ground-up rewrite, rather than an
evolution of icalendar.el? And how should I be thinking about the future
of icalendar.el?

Having written this code, I can now understand and appreciate some of
the code in icalendar.el better, but am still of the opinion that it is
too closely tied to diary import/export to be as useful to other parts
of Emacs (especially Org and Gnus) as it could be.

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
  - rename the user-facing functions
    (icalendar-{import/export}-file and their variants)
    to make clear that they are diary-specific
  - delete(?)/deprecate the old parsing code

But let me know if that is not in the cards.

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:

  - 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?
  - 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? ...?)
  - I haven't really thought about performance at all. My parser is
    definitely slower/more memory intensive than icalendar.el's at the moment
    (though hopefully this is mostly because it has more to offer). Are there
    any things I should already be thinking about to make it more efficient?

3) Likewise, part of my patch was icalendar-mode.el, and it would be
useful to have some feedback about the major mode:

  - whether an icalendar-mode is even a useful thing to have, and what
    could make it more useful
  - 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)
  - 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?)
  - 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)

> I can see already that your doc strings are not always according to
> our conventions (the first line must be a single complete sentence),
> and also the commit log messages need to be more detailed.  But these
> are aspects best deferred until you have a version close to the final
> one, now is too early for that.

OK, but that's good to keep in mind, thanks.

(Re: commit messages: should I be making commits in Emacs' ChangeLog
format as I go along, and post a whole new patch series here after
revisions? Or does it make more sense to squash everything at the end
into one or a few commits, and only format the log messages at that
point?)

> One other comment is to be sure the many cl-isms you use don't get in
> the way of debugging the code, since Edebug is known to provide weaker
> support for some cl-lib construct and cl-macs macros.  So if you find
> some macros or constructs that would make debugging harder, and can
> replace them with simpler code without sacrificing too much, that
> would be appreciated.

Yes, this is another thing I was wondering about, so thanks for bringing
it up. I have actually tried to keep cl-isms to a minimum: I am only
using cl-defmacro (for keyword arguments) and the cl type system (mostly
cl-deftype and cl-typep, since the parser needs some way of representing
the types defined by the iCalendar standard, and checking that values
conform to those types). So far this hasn't gotten in the way of
debugging for me, though I am not a heavy Edebug user. (The posted patch
also uses one cl-defstruct but my recent exchange with Ihor Radchenko on
emacs-devel has convinced me that I can move to using
org-element-ast.el, which will eliminate that.)

I am using read-symbol-shorthands to turn the CL-style "ical:" prefix
into "icalendar-", which to me is more readable, but that's easy to
change/eliminate if others don't like it.

Thanks!
Richard

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.