Make parsing and assumptions about UID/RECURRENCE-ID explicit #26
Conversation
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
rfc2822
force-pushed
the
explicit-parser
branch
2 times, most recently
from
ba1052c to
f8d18db
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request centralizes iCalendar parsing and makes UID/RECURRENCE-ID handling explicit across the library and tests.
- Introduces
ICalendarParserfor preprocessing and wrapping parse errors. - Adds
CalendarUidSplitterandAssociatedComponentsto group and validate events by UID and RECURRENCE-ID. - Refactors higher-level consumers (
ICalendar,Event,AndroidEvent) to use the new parser and splitter, and standardizes exception types.
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ICalendarParser.kt | New parser class applying ICalPreprocessor with explicit error wrapping. |
| CalendarUidSplitter.kt | Splits components by UID/RECURRENCE-ID and filters by highest sequence. |
| AssociatedComponents.kt | Data class enforcing main/exception UID constraints. |
| Ical4jHelpers.kt | Helper factories and extension properties for CalendarComponent. |
| ICalendar.kt | Switched to ICalendarParser and unified exception types. |
| Event.kt & AndroidEvent.kt | Updated to use new parser/splitter and InvalidLocalResourceException. |
| lib/src/test/... (multiple) | New unit tests covering parser, splitter, and associated components. |
| build.gradle.kts & libs.versions.toml | Added MockK dependency for new parser tests. |
Comments suppressed due to low confidence (1)
lib/src/main/kotlin/at/bitfire/synctools/icalendar/Ical4jHelpers.kt:18
- [nitpick] Add KDoc describing the purpose and expected usage of this helper function to improve discoverability and maintenance.
fun<T: CalendarComponent> componentListOf(vararg components: T) =
lib/src/main/kotlin/at/bitfire/synctools/icalendar/CalendarUidSplitter.kt
Show resolved
Hide resolved
sunkup
previously approved these changes
Jul 1, 2025
Member
sunkup
left a comment
sunkup
left a comment
There was a problem hiding this comment.
Took me a while to understand, but I think the functionality from the original Event.eventsFromReader is preserved and it's a lot cleaner and safer now!
lib/src/main/kotlin/at/bitfire/synctools/exception/InvalidResourceException.kt
Show resolved
Hide resolved
lib/src/test/kotlin/at/bitfire/synctools/icalendar/ICalendarParserTest.kt
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
For: bitfireAT/davx5-ose#1554
Currently iCalendar parsing and UID/RECURRENCE-ID splitting is done in the
Eventdata class, where it does not belong to. This PRICalendarParser(+ tests),CalendarUidSplitter(+ tests),AssociatedComponentsto associate a main event and its exceptions (+ tests).There's a FIXME in the code, but it keeps current behavior and fixing that is not in the scope of this PR. I just added the comment to mark that code as bad.
CC @sunkup: Please have a look and comment especially if the "whole thing" that it does is unclear. I tried to add as good KDoc as possible. I however understand that it's hard to review this PR when its taken out of the whole refactoring context.