Add iCalendar export support #2362
Conversation
Summary by CodeRabbit
WalkthroughThis pull request adds iCalendar export capability to the holidays library. A new Changes
Assessment against linked issues
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #2362 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 205 206 +1
Lines 13058 13134 +76
Branches 1869 1881 +12
=========================================
+ Hits 13058 13134 +76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
AUTHORS(1 hunks)holidays/ical.py(1 hunks)tests/test_ical.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/test_ical.py (1)
holidays/ical.py (2) (2)
ICalExporter(17-140)generate(118-140)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: Test build on windows-latest
🔇 Additional comments (21)
AUTHORS (1)
59-59: Welcome new author
Adding "Hiroki Kawahara" maintains consistency with the existing author list and properly acknowledges contribution.tests/test_ical.py (15)
22-26: Straightforward setup
ThesetUpmethod correctly prepares a valid holidays object for subsequent tests.
34-41: Timestamp test
Thoroughly validates the custom DTSTAMP logic.
42-47: Good version retrieval test
Ensures_version__is parsed correctly and assigned.
48-54: ValueError test
Properly checks handling of missing version string.
55-60: FileNotFoundError coverage
Protects against a nonexistentholidays/version.pyfile.
62-86: Comprehensive language handling
Tests confirm the fallback to "EN" and enforcement of two-letter codes.
87-97: Single holiday event
Validates that a single day event is properly generated.
106-118: Continuous span
Provides a good foundation for multi-day events testing, even if only single-day durations are currently used.
119-129: Non-continuous holidays
Ensures that holidays spanning disjoint dates are handled as separate events.
130-136: Localized text
Demonstrates correct use of non-English characters in theSUMMARYfield.
137-145: Empty holiday list
Confirms empty input still yields a valid base calendar structure.
146-176: Line folding check
Thorough coverage of splitting lines according to RFC 5545's 75-octet limit.
177-191: Very long holiday
Successfully tests handling of extended text and folding.
192-210: Return bytes option
Diligently verifies binary output, including non-ASCII characters.
211-221: Multiple fold passes
Guarantees that repeated line folding continues correctly.holidays/ical.py (5)
17-47: Graceful version handling
The constructor appropriately sets and checks the version, with robust error handling for missing files or data.
48-67: Language validator
The_validate_languagemethod ensures two-letter ISO 639-1 conformity, preventing unpredictable iCal behavior.
68-87: Line folding logic
Adheres to RFC 5545's 75-octet line limit, effectively handling splitting and indentation.
88-117: Event generation
Summaries, timestamps, and durations are handled well, ensuring each event is distinct via a UUID.
118-141: Calendar assembly
Comprehensively constructs a valid VCALENDAR, yielding either text or bytes as requested.
Co-authored-by: ~Jhellico <[email protected]> Signed-off-by: Panpakorn Siripanich <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
holidays/ical.py (1)
109-133: 🧹 Nitpick (assertive)Consider single-object returns for final output.
Instead of yielding line by line, some users prefer a single
.icsstring (or bytes) for direct export. This is a design choice. Offering an alternative method returning a fully joined CRLF-terminated string can be more convenient in some scenarios.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/ical.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI/CD
holidays/ical.py
[error] 16-16: ruff-format failed. 1 file reformatted, indicating formatting issues.
🔇 Additional comments (3)
holidays/ical.py (3)
1-15: File header and imports look fine.No issues found with the docstring or initial imports. The headers and license statements follow good practices.
20-38: Previous request to relocate or rename thereturn_bytesparameter.KJhellico previously suggested moving
return_bytesto thegenerate()method or introducing separate methods (as_string()andas_bytes()). This approach keeps the constructor cleaner and clarifies usage. Consider that if you prefer more flexible usage patterns.
59-78: Verify folding logic for multi-byte characters.Truncating based on
len(...encode("utf-8"))can cause splits within multi-byte sequences. Consider using a Unicode-aware approach to avoid mid-character breaks if your iCalendar data uses non-ASCII characters.Would you like assistance with a more robust line-folding method?
… class, `default_language` fallback now used when appropriate
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
holidays/ical.py(1 hunks)tests/test_ical.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/test_ical.py (1)
holidays/ical.py (2) (2)
ICalExporter(19-135)generate(112-135)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: Test build on windows-latest
🔇 Additional comments (17)
tests/test_ical.py (12)
1-13: No concerns in the header section.
14-27: MockHolidays usage is straightforward and enhances test clarity.
29-37: setUp method is well-structured.
The multiple country holiday objects help ensure broad coverage.
38-45: Basic calendar structure test is comprehensive.
Nice check for key lines: "BEGIN:VCALENDAR", "PRODID:", "VERSION:2.0", "END:VCALENDAR".
46-53: Timestamp override test is correct.
Verifies the custom DTSTAMP properly. Good coverage.
76-86: Single holiday event assertions look good.
87-122: Multiple holiday scenarios are well-covered.
Tests for same-date overlaps, continuous vs non-continuous dates, and empty sets.
123-153: Line folding edge-case tests are excellent.
Ensures compliance with the 75-octet rule, handling multi-step folding.
154-168: Test for very long holiday names is solid.
Impressive coverage for multi-byte characters and line folding.
169-181: Byte return tests are correct.
Verifies both ASCII & multi-byte data correctness.
182-188: CRLF line endings test confirms proper compliance with RFC 5545.
189-199: Multiple fold iterations test is robust.
Ensures no breakage when folding lines repeatedly.holidays/ical.py (5)
1-18: Imports and version references are straightforward.
No issues spotted.
19-41: Constructor logic is clear and flexible.
Theical_timestampfallback is useful. The approach for defaulting to "EN" language is sensible.
62-81: Line folding function is robust.
Watchposdecrement in rare cases of extremely large multi-byte lines, but tests cover typical usage thoroughly.
82-111: Event generation is clean and follows the iCalendar standard.
UID structure is logical, single-day duration is correct for now.
112-136: Calendar assembly is solid.
Uses CRLF, invests in a final join, returning string or bytes accordingly. This design looks good.
…69-1 code from Library of Congress
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
holidays/ical.py (1)
124-155: 🧹 Nitpick (assertive)Potential expansion for advanced iCal features.
The generation logic is solid. In the future, you may consider supporting additional fields likeLOCATIONorCATEGORIES, enabling more QoL features for end-users who need advanced calendar options.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
holidays/ical.py(1 hunks)tests/test_ical.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/test_ical.py (1)
Learnt from: PPsyrius
PR: vacanza/holidays#2362
File: tests/test_ical.py:54-75
Timestamp: 2025-03-22T09:31:16.025Z
Learning: The ICalExporter class in holidays/ical.py already handles empty language strings, likely treating them similar to None values which default to "EN".
🧬 Code Definitions (1)
tests/test_ical.py (1)
holidays/ical.py (2) (2)
ICalExporter(19-154)generate(113-154)
🔇 Additional comments (3)
tests/test_ical.py (3)
37-44: Comprehensive structure validation.
This test effectively verifies the essential iCalendar blocks, ensuring "BEGIN:VCALENDAR" and "END:VCALENDAR" are present alongside standard headers. Good coverage here.
92-97: Thorough check for multiple holidays on the same date.
This confirms that the exporter includes separate events for distinct holiday names falling on the same date, which is quite useful for real-world scenarios.
127-147: Solid line folding edge-case tests.
These tests comprehensively handle borderline lengths, ensuring compliance with RFC 5545. The approach covers exact, near-limit, and over-limit line lengths well.
|
I cannot even run |
|
Try rebuild package with |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
tests/test_ical.py (1)
434-443: 🧹 Nitpick (assertive)Consider using a context manager for UTF-8 filename test.
Similar to the earlier file test, using a context manager or temporary directory would ensure cleanup even if the test fails.
- def test_export_ics_with_utf8_name(self): - utf8_filename = "test_ปฏิทิน" - - self.th_exporter.export_ics(file_path=f"{utf8_filename}.ics") - self.assertTrue( - os.path.exists(f"{utf8_filename}.ics"), - "File should be created with special characters in the name.", - ) - os.remove(f"{utf8_filename}.ics") + def test_export_ics_with_utf8_name(self): + utf8_filename = "test_ปฏิทิน" + file_path = f"{utf8_filename}.ics" + + try: + self.th_exporter.export_ics(file_path=file_path) + self.assertTrue( + os.path.exists(file_path), + "File should be created with special characters in the name.", + ) + finally: + if os.path.exists(file_path): + os.remove(file_path)
♻️ Duplicate comments (1)
tests/test_ical.py (1)
80-88: 🧹 Nitpick (assertive)Consider expanding special character tests.
Your special character handling tests cover basic cases, but could benefit from testing more complex scenarios like newlines and multi-line holiday names.
def test_escape_character_holiday_names(self): # Add more edge cases for special characters + # NEWLINE character. + self._assert_special_char_handling("Holiday\nEvent", "Holiday\\nEvent") + + # TAB character. + self._assert_special_char_handling("Holiday\tEvent", "Holiday\\tEvent")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
tests/test_ical.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
tests/test_ical.py (5)
Learnt from: PPsyrius
PR: vacanza/holidays#2362
File: tests/test_ical.py:54-75
Timestamp: 2025-03-26T17:15:40.859Z
Learning: The ICalExporter class in holidays/ical.py already handles empty language strings, likely treating them similar to None values which default to "EN".
Learnt from: PPsyrius
PR: vacanza/holidays#2362
File: tests/test_ical.py:31-40
Timestamp: 2025-03-26T17:15:40.859Z
Learning: In the holidays library's `TestIcalExporter` class, initializing `ICalExporter` with `None` is intentional. Using a mock object instead breaks tests because the constructor performs attribute validation with `getattr(self.holidays, "language", None)` which returns a mock with a mock object instead of `None`, causing validation failures. The approach isolates file operation tests from iCal generation logic.
Learnt from: PPsyrius
PR: vacanza/holidays#2362
File: tests/test_ical.py:31-40
Timestamp: 2025-03-26T17:15:40.859Z
Learning: In the holidays library tests, `TestIcalExporter` initializes `self.exporter = ICalExporter(None)` intentionally and then mocks the `generate` method, which is a valid test pattern for isolating file operation tests from the actual iCal generation logic. Replacing `None` with a mock object breaks the tests.
Learnt from: PPsyrius
PR: vacanza/holidays#2362
File: tests/test_ical.py:270-283
Timestamp: 2025-03-26T17:15:40.859Z
Learning: The `export_ics` method in `ICalExporter` takes a string parameter `file_path` and doesn't use `Path` from `pathlib` for file operations.
Learnt from: PPsyrius
PR: vacanza/holidays#2362
File: tests/test_ical.py:270-283
Timestamp: 2025-03-24T04:30:41.443Z
Learning: The `export_ics` method in `ICalExporter` takes a string parameter `file_path` and doesn't use `Path` from `pathlib` for file operations.
🧬 Code Definitions (1)
tests/test_ical.py (2)
holidays/holiday_base.py (4)
HolidayBase(57-1242)_populate(870-892)_populate(1354-1357)_add_holiday(793-805)holidays/ical.py (3)
ICalExporter(27-230)generate(170-212)export_ics(214-230)
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: Test build on windows-latest
🔇 Additional comments (24)
tests/test_ical.py (24)
1-21: Clean and comprehensive header, imports and license information.The file starts with clear license information and includes all necessary imports for the test suite. This sets a good foundation for the comprehensive test file.
23-33: Well-designed test helper class.The
MockHolidaysclass provides a clean implementation for testing purposes, with a helpful custom method for adding test holidays. This approach allows for isolated and controlled testing of theICalExporterfunctionality.
35-39: Effective datetime mocking.The
MockDatetimeclass provides a clean way to ensure consistent timestamp generation during tests, which is crucial for predictable test results.
41-53: Test setup provides good coverage with different holiday datasets.The setup method initializes the test environment with multiple country-specific holiday datasets, which helps ensure the exporter works with various real-world data. I see from the learnings that initializing with
Noneand mocking the generate method is intentional for isolating file operation tests.
54-75: Thorough line length validation helper.This helper correctly validates that generated lines comply with RFC 5545 length requirements and properly checks continuation line formatting. The error messages are descriptive, which will help diagnose test failures.
89-111: Comprehensive language code validation.The language code validation methods properly check both valid and invalid language codes, ensuring the exporter handles language codes correctly according to ISO 639 standards.
113-131: Strong tests for language handling.These helper methods thoroughly test language handling in holiday summaries, including important edge cases like combined holiday lists with different languages.
132-150: Comprehensive calendar structure validation.The test properly validates all essential components of the iCalendar format, including both calendar and event elements. This ensures the exporter generates standards-compliant output.
151-166: Clean timestamp testing approach.Mocking the datetime class is an excellent way to ensure consistent timestamps in tests. The test verifies that all DTSTAMP fields contain the expected timestamp value.
167-204: Thorough language code validation tests.The tests cover a wide range of both valid and invalid language codes, ensuring robust validation according to the ISO 639 standards. Particularly good to see tests for edge cases like region subtags and invalid structures.
206-232: Comprehensive combined holiday list tests.These tests ensure that language handling works correctly when combining holiday lists from different countries with different language settings. The test cases cover all important combinations.
233-248: Good test for continuous multi-day holiday handling.The test validates that a single continuous multi-day holiday (Songkran) is correctly represented as a single event with appropriate duration rather than multiple events.
249-259: Thorough test for continuous holidays with overlap.This test verifies the correct handling of continuous holidays that overlap with other holidays, ensuring that complex holiday patterns are correctly represented in the output.
260-268: Strong test for non-continuous holidays.The test validates that non-continuous occurrences of the same holiday are correctly represented as separate events. The assertion that checks for at least two single-day events is particularly valuable.
269-280: Good combined holiday list generation test.This test confirms that duplicate holidays from different countries are correctly merged into a single event, avoiding redundant entries in the calendar.
281-306: Comprehensive character escaping tests.The tests cover various special characters that require escaping in iCalendar format, including the handling of semicolons as delimiters in HolidayBase.
307-311: Localized holiday name test.The test ensures that holiday names in different languages are correctly included in the output.
312-331: Thorough empty holidays test.The test confirms that when no holidays are present, a valid iCalendar structure is still generated without any events. The explicit checks for absent event components are particularly valuable.
332-368: Comprehensive line folding tests.These tests thoroughly validate that line folding works correctly for various content types, including ASCII, UTF-8, emojis, and complex Unicode characters. The edge case testing at exactly 75 octets is especially good.
369-372: Good byte output tests.The tests verify that the exporter can generate byte output for both ASCII and non-ASCII content.
373-381: Strong line ending validation.The test ensures that all lines end with the correct CRLF sequence as required by RFC 5545.
382-399: Thorough UID generation tests.The tests verify that unique UIDs are generated for each event and that they're not reused across multiple generations. This is important for calendar interoperability.
414-420: Good empty content test.The test properly verifies that attempting to export empty content raises an appropriate ValueError.
421-433: Good file overwrite test with validation.The test confirms that existing files are properly overwritten and contains an excellent verification that the new content is valid iCalendar data.
Co-authored-by: Arkadii Yakovets <[email protected]> Signed-off-by: Panpakorn Siripanich <[email protected]>
|
|
Er - is there a way to force GitHub action to run again somehow:
|
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
holidays/ical.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
holidays/ical.py (5)
<retrieved_learning>
Learnt from: KJhellico
PR: #2362
File: holidays/ical.py:73-95
Timestamp: 2025-03-26T17:15:40.859Z
Learning: The _fold_line method in the ICalExporter class in holidays/ical.py implements proper RFC 5545 line folding by using separate approaches for ASCII and non-ASCII text. The ASCII branch uses simple string slicing, while the non-ASCII branch uses a character-by-character approach that respects Unicode code point boundaries to prevent splitting characters across lines.
</retrieved_learning>
<retrieved_learning>
Learnt from: PPsyrius
PR: #2362
File: tests/test_ical.py:270-283
Timestamp: 2025-03-26T17:15:40.859Z
Learning: The export_ics method in ICalExporter takes a string parameter file_path and doesn't use Path from pathlib for file operations.
</retrieved_learning>
<retrieved_learning>
Learnt from: PPsyrius
PR: #2362
File: holidays/ical.py:176-180
Timestamp: 2025-03-26T17:15:40.859Z
Learning: The "EN" in the PRODID line of the iCalendar export (f"PRODID:-//Vacanza//Open World Holidays Framework v{self.holidays_version}//EN") should be kept as a fixed identifier by design, as agreed upon by the team, even though the actual holiday language may be different.
</retrieved_learning>
<retrieved_learning>
Learnt from: PPsyrius
PR: #2362
File: holidays/ical.py:53-80
Timestamp: 2025-03-26T17:15:40.859Z
Learning: The holidays library intentionally limits language code support to ISO 639-1 and ISO 639-2 standards (2-3 letter language codes with optional region) to maintain consistency with other modules like common_holidays and financial_holidays, rather than implementing full RFC 5646 compliance.
</retrieved_learning>
<retrieved_learning>
Learnt from: PPsyrius
PR: #2362
File: holidays/ical.py:27-52
Timestamp: 2025-03-27T04:46:17.044Z
Learning: In the ICalExporter class, the language tag (;LANGUAGE=) should only be included in the SUMMARY field when explicitly requested (show_language=True) and a valid language is available. The default behavior (show_language=False) doesn't include the language tag.
</retrieved_learning>
🔇 Additional comments (5)
holidays/ical.py (5)
28-41: Constructor design looks clear and succinct.The constructor properly initializes all members, handles language retrieval and raises an exception if
show_languageis requested without a configured language. Nicely done.
62-88: Structured and valid language validation.The pattern is sufficiently strict for ISO 639-1/2 compliance. The error message is also user-friendly. Great approach for enforcing expected language formats.
153-154: Smart escaping strategy for special characters.This usage aligns well with RFC 5545 guidelines. The code excludes semicolons, which is consistent with the library's delimiter usage. Nicely managed.
180-205: Efficient merging of continuous holidays.The loop merges consecutive holidays with the same name into a single multi-day event. This is a performance-friendly approach for everyday use cases.
212-229: Clear file export logic with straightforward error handling.Saving
.icsfiles with checks on empty content is a nice touch. Maintaining minimal side effects here ensures clarity. Looks good.
Proposed change
A continuation of @kawazap's work in #2261, based on RFC 5545
Now with the following changes:
AUTHORSlist.CALSCALE,DTSTAMP,UIDPRODIDto includeholidaysversion..generatemethod now enforcesCRLFfile ending,return_bytesis moved there instead.VEVENT, utilizingDURATIONparameter.LANGUAGE=toSUMMARY, enabled byshow_languageparam..export_icsmethod to actually export ready-to-use iCalendar files.HolidaySumnow keepslanguageattribute if both Holidays object has the samelanguage.Resolves #2179.
Resolves #2261.
Type of change
holidaysfunctionality in general)Checklist
make check, all checks and tests are green