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

From: Yuchen Pei <[email-masked]>
Date: Thu, 26 Aug 2021 16:49:29 +1000

Thanks for the comments.
Eli Zaretskii <[email-masked]> writes:

>> From: Yuchen Pei <[email-masked]>
>> 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>