docs: clarify optional properties in Alarm.new() docstring#1492
docs: clarify optional properties in Alarm.new() docstring#1492Aman-0402 wants to merge 1 commit into
Conversation
|
Profile summary: Full profile |
Documentation build overview
3 files changed± 404.html± reference/api/icalendar.cal.alarm.html± _modules/icalendar/cal/alarm.html |
SashankBhamidi
left a comment
There was a problem hiding this comment.
Thanks for picking up issue #1473 and taking a stab at the docstring.
Two things need addressing before this can merge:
First, there is no news fragment in news/. The contributing guidelines require one for documentation changes, and the checklist in your PR description has it unchecked. A news/1492.documentation file with a one-line summary and your GitHub handle would do it.
Second, and more importantly: issue #1473 specifically asked for a comparison against RFC 5545 §3.6.6 to understand which properties are actually required versus optional. This PR adds "Optional." uniformly to all eight parameters, but DESCRIPTION is REQUIRED (not optional) for DISPLAY and EMAIL alarm action types per the RFC. The function doesn't have an action parameter, so at this level "optional in Python" is technically accurate since you can construct an Alarm() without one, but I'd like to understand whether you checked the RFC and made a conscious call here, or whether the "Optional." label was added without that context. If the former, adding a note like "Required for DISPLAY and EMAIL action types per :rfc:`5545` §3.6.6" on the description line would give readers the RFC context the issue was asking for.
Procedural note: if any part of this was AI-assisted, please follow the Responsible AI use policy. Standard reminder.
| uid: The :attr:`uid` of the alarm. | ||
| attendees: Optional. The :attr:`attendees` of the alarm. | ||
| concepts: Optional. The :attr:`~icalendar.cal.component.Component.concepts` of the alarm. | ||
| description: Optional. The :attr:`description` of the alarm. |
There was a problem hiding this comment.
DESCRIPTION is listed as unconditionally optional, but RFC 5545 §3.6.6 says it is REQUIRED for DISPLAY and EMAIL action types. Since Alarm.new() doesn't take an action parameter, callers have no way to know this constraint from the docstring alone. Did you check the RFC when writing this? If so, it would be worth adding a note to the description line so users building a DISPLAY or EMAIL alarm know they need to set it. Something like the following would cover it without making the param doc overly long.
| description: Optional. The :attr:`description` of the alarm. | |
| description: The :attr:`description` of the alarm. Required for DISPLAY and EMAIL action types per :rfc:`5545#section-3.6.6`. |
There was a problem hiding this comment.
@stevepiercy Is it possible to tag the section in the rfc?
There was a problem hiding this comment.
@SashankBhamidi thanks for the review. Under General cross-references, there are examples of how to link to a section of an RFC.
Docstring structure also describes how to write docstrings.
There was a problem hiding this comment.
@SashankBhamidi thanks for the review. Under General cross-references, there are examples of how to link to a section of an RFC.
@stevepiercy I've edited the suggestion, does that look good?
There was a problem hiding this comment.
Yes, unless ruff complains about line length.
Linked issue
Closes #1473
Description
This PR adds "Optional." to each parameter description in the
Alarm.new()docstring to clarify that these parameters are optional.Checklist
/news, following the instructions in Change log entry format.Additional information
Docs-only change — no functional code modified.