Merged
Conversation
Contributor
Summary by CodeRabbit
WalkthroughConverts ICalExporter._generate_event to an Iterable generator (yields lines) and emits a single-category Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
holidays/ical.py (1)
141-176:⚠️ Potential issue | 🟠 MajorUse RFC 5545 standard
CATEGORIESproperty.
CATEGORYviolates RFC 5545 §3.8.1.2, which specifiesCATEGORIESas the property name for calendar event categories. Non-compliant readers may silently ignore the output. Update the property name and the corresponding test assertions.Changes required
holidays/ical.py:
if len(self.holidays.categories) == 1: - yield f"CATEGORY:{next(iter(self.holidays.categories)).upper()}" + yield f"CATEGORIES:{next(iter(self.holidays.categories)).upper()}"tests/test_ical.py (lines 464-476):
def test_holidays_category(self): single_category_holidays = country_holidays( "US", years=2024, categories=UNOFFICIAL, language="en_US" ) output = ICalExporter(single_category_holidays).generate() - self.assertIn("CATEGORY:UNOFFICIAL", output) + self.assertIn("CATEGORIES:UNOFFICIAL", output) multi_category_holidays = country_holidays( "US", years=2024, categories=(HALF_DAY, UNOFFICIAL), language="en_US" ) output = ICalExporter(multi_category_holidays).generate() - self.assertNotIn("CATEGORY:HALF_DAY", output) - self.assertNotIn("CATEGORY:UNOFFICIAL", output) + self.assertNotIn("CATEGORIES:HALF_DAY", output) + self.assertNotIn("CATEGORIES:UNOFFICIAL", output)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@holidays/ical.py` around lines 141 - 176, The VEVENT generator uses the non-standard PROPERTY name "CATEGORY"; change the property to the RFC 5545-compliant "CATEGORIES" in _generate_event (replace the yield that emits f"CATEGORY:..." with f"CATEGORIES:...") so single-category events use "CATEGORIES:<NAME>". Also update the unit tests that assert the old "CATEGORY" header (tests that reference the literal "CATEGORY" in iCal output) to expect "CATEGORIES" instead and keep the existing value formatting/uppercasing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@holidays/ical.py`:
- Around line 141-176: The VEVENT generator uses the non-standard PROPERTY name
"CATEGORY"; change the property to the RFC 5545-compliant "CATEGORIES" in
_generate_event (replace the yield that emits f"CATEGORY:..." with
f"CATEGORIES:...") so single-category events use "CATEGORIES:<NAME>". Also
update the unit tests that assert the old "CATEGORY" header (tests that
reference the literal "CATEGORY" in iCal output) to expect "CATEGORIES" instead
and keep the existing value formatting/uppercasing behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev #3290 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 307 307
Lines 18387 18396 +9
Branches 2353 2354 +1
=========================================
+ Hits 18387 18396 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
arkid15r
previously approved these changes
Feb 25, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_ical.py`:
- Around line 413-425: The test reads files using bare open() calls (building
content_1 and content_2) which can leak file descriptors; change both reads to
use context managers (with open(file_path_1) as f: and with open(file_path_2) as
f:) and iterate f.readlines() or the file iterator while filtering out lines
starting with "UID:" so that the file handles are closed immediately after
reading; keep the existing logic that strips line endings and the call to
self.us_exporter.save_ics(file_path=file_path_2).
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_ical.py`:
- Around line 413-427: The comparison in tests/test_ical.py currently filters
only UID lines when building content_1 and content_2, which makes the assertion
flaky due to differing DTSTAMP values; update the list comprehensions that build
content_1 and content_2 (the blocks surrounding file_path_1.read_text() and
file_path_2.read_text() used with self.us_exporter.save_ics) to also skip lines
starting with "DTSTAMP:" so both UID and DTSTAMP are ignored before asserting
equality.
arkid15r
approved these changes
Feb 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Update iCal exporter:
pathlib.PathType of change
holidaysfunctionality in general)Checklist
make checklocally; all checks and tests passed.