Thanks for the review.

Robert Pluim <rpluim@gmail.com> writes:

> Now that you have the bug number, please put (Bug#72831) somewhere in
> the commit message.

Updated in the patch attached. 

> I think a change this size requires copyright assigment, which I donʼt
> know if youʼve done.

Copyright assignment is done. I wrote some prior contributions to org-mode.

> I think that if youʼre making the same comment for the three functions
> you can put all three in one set of (), separated by commas.

Updated.

>     >> @@ -319,6 +319,10 @@ gnus-icalendar-event--build-reply-event-body
>     >> (if (string-match "^[^:]+:" line)
>     >> (replace-match (format "\\&%s: " summary-status) t nil line)
>     >> line))
>     >> +         (update-comment
>     >> +           (line)
>     >> +           (if comment (format "COMMENT:%s" comment)
>     >> +             line))
>     >> (update-dtstamp ()
>     >> (format-time-string "DTSTAMP:%Y%m%dT%H%M%SZ" nil t))
>     >> (attendee-matches-identity
>     >> @@ -341,6 +345,7 @@ gnus-icalendar-event--build-reply-event-body
>     >> (cond
>     >> ((string= key "ATTENDEE") (update-attendee-status line))
>     >> ((string= key "SUMMARY") (update-summary line))
>     >> +		     ((string= key "COMMENT") (update-comment line))
>     >> ((string= key "DTSTAMP") (update-dtstamp))
>     >> ((member key '("ORGANIZER" "DTSTART" "DTEND"
>     >> "LOCATION" "DURATION" "SEQUENCE"
>     >> @@ -363,12 +368,17 @@ gnus-icalendar-event--build-reply-event-body
>     >> attendee-status user-full-name user-mail-address)
>     >> reply-event-lines))
>     >> 
>     >> +      ;; add comment line if not existing
>     >> + (when (and comment (not (gnus-icalendar-find-if (lambda (x)
>     >> (string-match "^COMMENT" x))
>     >> + reply-event-lines)))
>     >> +        (push (format "COMMENT:%s" comment) reply-event-lines))
>     >> +
>
> So if the event was sent with a COMMENT the receiver canʼt add their
> own? That doesnʼt match my conception of 'reply with comment'. Iʼm
> hazy on whatʼs exactly allowed in ical, can you have more than one
> COMMENT line? Or we could combine the comments?

If it already exists it is replaced by the prior
>    >> ((string= key "COMMENT") (update-comment line))
Just if it does not exist the `(string= key "COMMENT")` never matches and we have to add the field.

>     >> -(defun gnus-icalendar-event-reply-from-buffer (buf status identities)
>     >> +(defun gnus-icalendar-event-reply-from-buffer (buf status
>     >> identities &optional comment)
>     >> "Build a calendar event reply for request contained in BUF.
>     >> The reply will have STATUS (`accepted', `tentative' or  `declined').
>     >> The reply will be composed for attendees matching any entry
>
> Could you describe the COMMENT arg in the docstring?

Updated.

>     >> @@ -1009,25 +1019,37 @@ gnus-icalendar-save-event
>     >> (when data
>     >> (gnus-icalendar-save-part data))))
>     >> 
>     >> -(defun gnus-icalendar-reply-accept ()
>     >> -  "Accept invitation in the current article."
>     >> +(defun gnus-icalendar-reply-accept (&optional comment-p)
>     >> +  "Accept invitation in the current article.
>     >> +
>     >> +With a prefix `\\[universal-argument]' prompt for a comment to include
>     >> +in the reply."
>
> I think we tend to word this as
>
> "Optional argument COMMENT-P (interactively the prefix argument) means
> prompt for a comment to include in the reply."

Updated using `\\[universal-argument]'. Or does that not matter here and just "prefix argument" would be fine?

> Ideally youʼd add test cases for this to
> "test/lisp/gnus/gnus-icalendar-tests.el". But thatʼs not mandatory.

I looked into it and will add a few tests for accepting/declining events with and without comments. I will followup on this in a couple days.
It might also make sense to extend the parsing of events to recognize comments (and potentially display them inside emacs.


I also missed to update the interactive argument for `gnus-icalendar-reply-accept` and `gnus-icalendar-reply-tentative`, which is now fixed in the updated patch.


Best,