bug#50195: [PATCH] Adding diary-offset to diary-lib.el

[Yuchen Pei]

Thanks for the comments.
Eli Zaretskii <eliz@gnu.org> writes:

> > From: Yuchen Pei <hi@ypei.me>
> > Date: Wed, 25 Aug 2021 13:46:48 +1000
> >
> > See below my first patch to Emacs.  The copyright assignment 
> > has 
> > already been done BTW.
>
> Congrats, and welcome aboard.
>
> > Let me know what you think.
>
> Some minor comments below.
>
> > Would you like me to add some tests?
>
> Adding more tests is always welcome, thanks.

All diary sexp tests are in icalendar tests, but diary-offset does 
not easily translate to icalendar events (exporting to icalendar 
for this sexp is rather complicated if not impossible as it 
applies on top of another arbitrary sexp), so I am just adding a 
simple no-op-like test.

> > Subject: [PATCH] Adding diary-offset, a diary-sexp offsetting 
> > another
> >  diary-sexp.
>
> "git am" uses the Subject for the heading line, and this Subject 
> is
> too long for that.  Please consider making it shorter.

Done.

> > A bit like diary-remind, as a diary-sexp rather than reminder, 
> > and
> > also support both positive and negative offsets.
> >
> > This is useful when for example your organization has a 
> > committee
> > meeting two days after every monthly meeting which takes place 
> > on the
> > third Thursday, or if you would like to attend a virtual 
> > meeting
> > scheduled in a different timezone causing a difference in the 
> > date.
>
> The commit log message should include a ChangeLog-style 
> description of
> the files and functions where you made the changes.  See 
> CONTRIBUTE
> for more details about the format we prefer.
>
> Also, please in the next version include the bug number as part 
> of the
> log message.

Done.

> > --- a/doc/emacs/calendar.texi
> > +++ b/doc/emacs/calendar.texi
> > @@ -1363,6 +1363,20 @@ Special Diary Entries
> >  Thursday of January, February, and March.  If the month is 
> >  @code{t}, the
> >  entry applies to all months of the year.
> >  
> > +@findex diary-offset
> > +@example
> > +%%(diary-offset '(diary-float t 3 4) 2) Monthly post-event 
> > committee meeting
>
> That line is too long, and will overflow the page width in the 
> printed
> version of the manual.  Please break it in two.

Done. I made it shorter :)

> > +@noindent
> > +This entry applies to the Saturday after the third Thursday of 
> > each
> > +month.  The 2 specifies number of days after when the sexp
> > +@samp{'(diary-float t 3 4)} would evaluate to @code{t}.  This 
> > is
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This should be in @code, not @samp.  I'd also wrap it in @w{..}, 
> so
> that it doesn't get broken between two lines.

Done.

> > +(defun diary-offset (sexp days)
> > +  "Offsetted diary entry.
>
> The first line of a doc string should preferably mention the
> arguments, but without becoming too long, so it could still fit 
> on a
> single line.

Done.

> > +  (with-no-warnings (defvar date) (defvar entry))
>
> Why did you need this?

The sexp diary-offset itself requires the date supplied by 
diary-sexp-entry.  The sexp passed to diary-offset will ask for 
both date and entry.  Removed (defvar entry) as this is not needed 
by diary-offset.  After the removel I tested it in org mode and 
org agenda and it works.

> > +  (integerp days)
>
> Isn't it better to use an assertion?

Done.  Added a user-error statement like in diary-cyclic.

> Thanks.


--
Best,
Yuchen

PGP Key: 47F9 D050 1E11 8879 9040  4941 2126 7E93 EF86 DFD0
          <https://ypei.me/assets/ypei-pubkey.txt>

signature.asc
Description: PGP signature