Add Alarm.new_display(), new_audio(), and new_email() factory methods by klouds27 · Pull Request #1417 · collective/icalendar

klouds27 · 2026-05-25T17:44:49Z

Closes #865.

What

Adds three class methods to Alarm for creating the three standard VALARM action types defined in RFC 5545 §3.6.6:

Alarm.new_display(description, trigger, ...) — ACTION:DISPLAY
Alarm.new_audio(trigger, attach=None, ...) — ACTION:AUDIO
Alarm.new_email(summary, description, trigger, attendees, ...) — ACTION:EMAIL

Each method sets ACTION and TRIGGER (both required by RFC 5545 for every alarm type) and enforces the action-specific required fields, raising InvalidCalendar on missing or invalid inputs. DURATION and REPEAT are accepted as optional paired parameters by all three methods and raise InvalidCalendar when only one is supplied.

Example

from datetime import timedelta
from icalendar import Alarm, vCalAddress

# Display alarm — 15 minutes before event start
alarm = Alarm.new_display(
    description="Team meeting in 15 minutes",
    trigger=timedelta(minutes=-15),
)

# Audio alarm with a custom sound file
alarm = Alarm.new_audio(
    trigger=timedelta(minutes=-5),
    attach="ftp://example.com/pub/sounds/bell-01.aud",
)

# Email alarm sent to two recipients
alarm = Alarm.new_email(
    summary="Meeting reminder",
    description="Your meeting starts in 30 minutes.",
    trigger=timedelta(minutes=-30),
    attendees=[
        vCalAddress("mailto:a@example.com"),
        vCalAddress("mailto:b@example.com"),
    ],
)

Tests

25 unit tests in src/icalendar/tests/attr/test_alarm_factory_methods.py covering:

Correct ACTION value for each method
Required property setters (description, summary, trigger, attendees)
Optional ATTACH and DURATION/REPEAT pairing
InvalidCalendar raised for each missing required field
InvalidCalendar raised when DURATION and REPEAT are not supplied together

📚 Documentation preview 📚: https://icalendar--1417.org.readthedocs.build/en/1417/

read-the-docs-community · 2026-05-25T17:45:43Z

Documentation build overview

📚 icalendar | 🛠️ Build #33758737 | 📁 Comparing 34bbe10 against latest (f035efe)

🔍 Preview build

91 files changed · + 2 added · ± 89 modified

+ Added

± Modified

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds convenience factory constructors for RFC 5545 VALARM components and accompanying tests/docs to make alarm creation easier and less error-prone.

Changes:

Add Alarm.new_display(), Alarm.new_audio(), and Alarm.new_email() factory methods with validation and docstrings.
Add pytest coverage for the new factory methods and validation behavior.
Add a news fragment announcing the new public APIs.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
src/icalendar/cal/alarm.py	Introduces three new `Alarm` factory classmethods (DISPLAY/AUDIO/EMAIL) with parameter validation and docs.
src/icalendar/tests/attr/test_alarm_factory_methods.py	Adds tests asserting correct properties and validation errors for the new factory methods.
news/865.feature	Documents the addition of the new factory methods.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

stevepiercy · 2026-05-26T11:06:04Z

@klouds27 would you please respond to the comments from copilot that you requested, and update your branch to main? I'll review separately. Thank you!

stevepiercy

This is excellent work. Thank you for improving icalendar! I have a few suggestions for you to consider. Would you please take care?

I also would like a second pair of eyes from a maintainer. @SashankBhamidi @niccokunzmann @angatha

Each method sets ACTION and TRIGGER (required by RFC 5545 §3.6.6) and validates that all action-specific required fields are present, raising InvalidCalendar on missing inputs. DURATION and REPEAT must be supplied together or not at all. Covered by 25 unit tests.

to_ical() sorts properties alphabetically; the docstring examples for new_audio and new_email assumed insertion order, causing doctest failures on Python 3.14. Updated expected output to match sorted order. Also clarified that absolute trigger accepts any datetime (UTC recommended but not enforced), and added the missing repeat-without-duration tests for new_audio and new_email.

- Remove positional-only `/` separator from new_display, new_audio, and new_email so all parameters are keyword-passable - Fix news/865.feature to use :rfc:`5545#section-3.6.6` markup - Replace `e.g.` with `such as` in attach/attendees docstrings - Fix vCalAddress cross-reference to use full module path - Simplify uid docstring to `or None` (uid is optional, not auto-generated) Signed-off-by: klouds27 <adalwolf@gmail.com>

klouds27 · 2026-05-26T21:03:10Z

Addressed all review comments:

Removed the positional-only / separator from new_display, new_audio, and new_email — all parameters are now explicitly keyword-passable
Updated news/865.feature to use :rfc:\5545#section-3.6.6`` markup
Replaced e.g. / i.e. with such as in attach and attendees docstrings
Fixed vCalAddress cross-reference to use the full module path icalendar.prop.cal_address.vCalAddress
Simplified all three uid docstring lines to or \`None```
The UTC trigger wording was already updated in the previous commit (changed to "recommend UTC-aware")
The repeat-without-duration tests were already added in the previous commit
Rebased onto current main

stevepiercy

This looks good to me. Since this is a new feature, I'd like to get a maintainer to review as well. @niccokunzmann @SashankBhamidi @angatha would you please do the honors?

Thank you!

coveralls · 2026-05-27T04:54:58Z

Coverage Report for CI Build 26491460473

Coverage increased (+0.01%) to 97.85%

Details

Coverage increased (+0.01%) from the base build.
Patch coverage: 2 uncovered changes across 1 file (152 of 154 lines covered, 98.7%).
No coverage regressions found.

Uncovered Changes

File	Changed	Covered	%
src/icalendar/cal/alarm.py	60	58	96.67%
Total (2 files)	154	152	98.7%

Coverage Regressions

No coverage regressions found.

Coverage Stats


Relevant Lines:	12879
Covered Lines:	12607
Line Coverage:	97.89%
Relevant Branches:	794
Covered Branches:	772
Branch Coverage:	97.23%
Branches in Coverage %:	Yes
Coverage Strength:	2.93 hits per line

💛 - Coveralls

SashankBhamidi

Nice work. The factory methods are RFC-correct.

A few things to address before merge. Findings 1, 3, 4, 6 are mechanical. Finding 2 is a design question I'd like to revisit with @niccokunzmann when he's back mid-June, @angatha curious if you have thoughts in the meantime.

Procedural note: if any part of this was AI-assisted, please follow the Responsible AI use policy. Standard reminder.

SashankBhamidi · 2026-05-27T09:24:05Z

@klouds27 When addressing each finding, please mark the conversation as resolved with a short reply, either confirming the fix in a specific commit, or noting why it's being deferred/skipped if applicable.

Same goes for the Copilot review you requested earlier. Helps keep the review state clean. Thanks!

- Extract repeated duration/repeat validation to _apply_duration_repeat helper - Reject empty string for new_audio attach parameter - Align new_display trigger docstring wording with new_audio and new_email - Add Event.new() integration example to new_display docstring Signed-off-by: klouds27 <adalwolf@gmail.com>

klouds27 · 2026-05-27T22:33:52Z

Addressed the mechanical findings: extracted repeated duration/repeat validation to a _apply_duration_repeat helper (finding 3), new_audio now rejects empty string attach the same as None (finding 4), aligned new_display trigger docstring to "recommend UTC-aware" to match the other two methods (finding 6), added an Event.new() integration example to new_display (finding 1).

Finding 2 (factory delegation to Alarm.new()) noted, happy to revisit when @niccokunzmann is back. Finding 5 (repeat=0 behavior) not yet addressed, let me know if you'd like it coerced or just documented.

angatha · 2026-05-28T05:06:37Z

About finding 2, personally I would add the parameters from Alarm.new as additional Parameters with the same default values and call Alarm.new instead of cls() and would make a comment at Alarm.new that the factories need to be updated as well. The factories get corresponding notes as well. This gives a complete API and everyone touching the API knows what also needs to be updated.

baptx · 2026-05-28T10:40:02Z

A property to get the action value should also be added:
#1421

niccokunzmann

Thank you so much for all that work! Let me know if you need anything. I added a few ideas to fit it into the existing structure of the package.

- new_audio: accept bytes for inline binary attachments (vBinary) - new_email: accept single vCalAddress or str for attendees/attachments - parametrize duration/repeat validation tests across all three factory methods Signed-off-by: klouds27 <adalwolf@gmail.com>

klouds27 · 2026-07-07T10:16:28Z

documented links, related_to, refids, and concepts in all three docstrings. on the guard style: if not description is deliberate — an empty string is semantically invalid for a description, so truthiness is the right check. trigger is None is explicit because None is the only invalid value there, a zero-length timedelta would be valid.

niccokunzmann

Hi, thanks for the PR! This looks really good to me. The only thing I would request is that the tests at the bottom of the file check the values, so the edge case is properly covered.

Checks that attendee addresses, attachment URIs, and binary attach data match the inputs, not just that the properties exist or have the right type. Also moves the three edge-case tests into their respective sections and lifts the vBinary import to module level. Signed-off-by: klouds27 <adalwolf@gmail.com>

klouds27 · 2026-07-10T11:24:54Z

added value assertions to all five tests that were checking type or presence only. the three follow-up items you flagged (ACTION attribute, attendees accepting str, uid/links/concepts coverage) can be opened as issues after merge.

niccokunzmann · 2026-07-15T16:45:32Z

@angatha @SashankBhamidi please.have another look and choose which chnages are essential.

niccokunzmann · 2026-07-15T16:46:41Z

TODO as follow-up:

Add Alarm.new_display(), new_audio(), and new_email() factory methods #1417 (comment)
Add Alarm.new_display(), new_audio(), and new_email() factory methods #1417 (comment)
Add Alarm.new_display(), new_audio(), and new_email() factory methods #1417 (comment)
validate repeat >= 0 Add Alarm.new_display(), new_audio(), and new_email() factory methods #1417 (comment)
Create an issue that the attendees can also be set to a list/SEQUENCE of strings with and without mailto:, new() methods need to be adjusted.
Complete the TODOs in Make all properties available to avoid additional loop #1421 (comment).

stevepiercy · 2026-07-15T21:48:55Z

@niccokunzmann @SashankBhamidi @angatha please resolve unresolved conversations that should be resolved in this issue to allow auto-merge when enabled.

SashankBhamidi · 2026-07-17T12:02:17Z

@klouds27,
One last thing before I re-approve.
Can you remove the section divider comments from the test file? The test names already make the grouping clear.

If anyone else gets to this before me, feel free to merge without my approval once @klouds27 has addressed this.

niccokunzmann

Removing the comments in the test files.

collective#1417 (comment) Co-authored-by: Nicco Kunzmann <niccokunzmann@rambler.ru>

The changes requested have been applied.

`Alarm.new_display()`, `Alarm.new_audio()` and `Alarm.new_email()` were added in collective#1417, but their optional parameters and `Parameters` entries did not follow the alphabetical order that `Alarm.new()` establishes, and none of them carried a `versionadded` admonition. - Order the optional parameters of the three factory methods alphabetically, matching `Alarm.new()`. The required positional parameters keep their existing order, so no call site breaks. - Order the `Parameters` entries in the docstrings the same way. - Add a `.. versionadded:: 7.3.0` admonition to each factory method, which was overlooked in collective#1417. - Add tests that assert the optional parameters of all four factory methods are in alphabetical order, and that `links`, `related_to`, `refids`, `concepts` and `uid` are forwarded to `Alarm.new()`. The three factory methods are not part of any released tag yet, so reordering their keyword parameters is not a breaking change. Fixes collective#1591 AI disclosure: I used Claude Opus 4.5 to help draft this change and its tests. Prompt: work on collective/icalendar issue collective#1591, verify which of its checklist items are actually still open on current main, align the optional parameters and docstring `Parameters` order of the three Alarm factory methods with `Alarm.new()`, add the missing `versionadded` admonitions, and add tests that fail before the change. Output: this commit. I reviewed the result, confirmed the new tests fail on unpatched main and pass with the change, and validated it locally with the full pytest suite (17837 passed), `pytest src/icalendar/tests/test_with_doctest.py` (368 passed) and `ruff check` / `ruff format`.

Copilot AI review requested due to automatic review settings May 25, 2026 17:44

klouds27 requested review from SashankBhamidi, angatha, niccokunzmann and stevepiercy as code owners May 25, 2026 17:44

Copilot AI reviewed May 25, 2026

View reviewed changes

Comment thread src/icalendar/cal/alarm.py

Comment thread src/icalendar/cal/alarm.py Outdated

Comment thread src/icalendar/cal/alarm.py Outdated

stevepiercy requested changes May 26, 2026

View reviewed changes

Comment thread news/865.feature Outdated

Comment thread src/icalendar/cal/alarm.py Outdated

Comment thread src/icalendar/cal/alarm.py Outdated

Comment thread src/icalendar/cal/alarm.py Outdated

Comment thread src/icalendar/cal/alarm.py Outdated

Comment thread src/icalendar/cal/alarm.py Outdated

klouds27 added 3 commits May 26, 2026 23:02

klouds27 force-pushed the fix/865-valarm-factory-methods branch from 78eabac to 2f4d232 Compare May 26, 2026 21:03

stevepiercy mentioned this pull request May 26, 2026

Make all properties available to avoid additional loop #1421

Open

stevepiercy previously approved these changes May 27, 2026

View reviewed changes

Merge branch 'main' into fix/865-valarm-factory-methods

b76c72b

stevepiercy assigned niccokunzmann, angatha and SashankBhamidi May 27, 2026

SashankBhamidi previously requested changes May 27, 2026

View reviewed changes

klouds27 dismissed stevepiercy’s stale review via 11c5c52 May 27, 2026 22:31

niccokunzmann requested changes Jun 4, 2026

View reviewed changes

Comment thread src/icalendar/cal/alarm.py Outdated

Comment thread src/icalendar/cal/alarm.py Outdated

Comment thread src/icalendar/cal/alarm.py Outdated

Comment thread src/icalendar/cal/alarm.py Outdated

Comment thread src/icalendar/tests/attr/test_alarm_factory_methods.py Outdated

niccokunzmann requested changes Jul 7, 2026

View reviewed changes

niccokunzmann self-requested a review July 15, 2026 16:42

niccokunzmann previously approved these changes Jul 15, 2026

View reviewed changes

niccokunzmann requested a review from SashankBhamidi July 15, 2026 16:44

stevepiercy previously approved these changes Jul 15, 2026

View reviewed changes

Merge branch 'main' into fix/865-valarm-factory-methods

efad8ab

angatha previously approved these changes Jul 20, 2026

View reviewed changes

Comment thread src/icalendar/cal/alarm.py

Comment thread src/icalendar/cal/alarm.py Outdated

niccokunzmann reviewed Jul 25, 2026

View reviewed changes

Comment thread src/icalendar/tests/attr/test_alarm_factory_methods.py Outdated

Comment thread src/icalendar/tests/attr/test_alarm_factory_methods.py Outdated

Comment thread src/icalendar/tests/attr/test_alarm_factory_methods.py Outdated

Apply suggestions from code review

8a61b08

collective#1417 (comment) Co-authored-by: Nicco Kunzmann <niccokunzmann@rambler.ru>

niccokunzmann dismissed stale reviews from angatha, stevepiercy, and themself via 8a61b08 July 25, 2026 19:15

Merge branch 'main' into fix/865-valarm-factory-methods

34bbe10

niccokunzmann enabled auto-merge July 25, 2026 19:16

niccokunzmann approved these changes Jul 25, 2026

View reviewed changes

niccokunzmann mentioned this pull request Jul 25, 2026

consolidate parameters and documentation for Alarm factory methods #1591

Closed

5 tasks

niccokunzmann merged commit 4bb851e into collective:main Jul 25, 2026
46 checks passed

This was referenced Jul 25, 2026

Make sure all factory methods' parameters are covered by tests #1592

Open

TDD: Allow passing emails to the attendees property #1593

Closed

Validate Alarm repeat when setting #1594

Closed

Solaris-star mentioned this pull request Jul 27, 2026

docs(alarm): consolidate parameters and docs of Alarm factory methods #1603

Merged

5 tasks

Uh oh!

Uh oh!

Conversation

klouds27 commented May 25, 2026 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Example

Tests

Uh oh!

read-the-docs-community Bot commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Documentation build overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevepiercy commented May 26, 2026

Uh oh!

stevepiercy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klouds27 commented May 26, 2026

Uh oh!

stevepiercy left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 27, 2026

Coverage Report for CI Build 26491460473

Coverage increased (+0.01%) to 97.85%

Details

Uncovered Changes

Coverage Regressions

Coverage Stats

💛 - Coveralls

Uh oh!

SashankBhamidi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SashankBhamidi commented May 27, 2026

Uh oh!

klouds27 commented May 27, 2026

Uh oh!

angatha commented May 28, 2026

Uh oh!

baptx commented May 28, 2026

Uh oh!

niccokunzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klouds27 commented Jul 7, 2026

Uh oh!

niccokunzmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

klouds27 commented May 25, 2026 •

edited by github-actions Bot

Loading

read-the-docs-community Bot commented May 25, 2026 •

edited

Loading

niccokunzmann commented Jul 15, 2026 •

edited

Loading

stevepiercy commented Jul 15, 2026 •

edited

Loading