Skip to content

Conversation

@rfc2822
Copy link
Member

@rfc2822 rfc2822 commented Jul 7, 2025

Depends on #36

This PR extracts Event builder (build…) and processor (populate…) logic to dedicated classes, without changing the logic itself.

  • Move builder/processor logic to separate classes.
  • Move tests.

The interesting part happens in AndroidEvent. The populate/build methods are only slightly adapted, for instance by introducing a to: Event argument for populate. The logic of building/populated has not been touched.

The huge number of lines comes from moving the methods and the tests.

PS: Changed some tests to become more independent of each other, for instance by using the class name in test accounts.

@rfc2822 rfc2822 marked this pull request as ready for review July 7, 2025 10:59
@rfc2822 rfc2822 marked this pull request as draft July 7, 2025 10:59
@rfc2822 rfc2822 self-assigned this Jul 7, 2025
@rfc2822 rfc2822 added the refactoring Quality improvement of existing functions label Jul 7, 2025
@rfc2822 rfc2822 force-pushed the introduce-mapping branch from 33435b1 to a937e5d Compare July 7, 2025 16:23
@rfc2822 rfc2822 requested a review from Copilot July 7, 2025 16:29
@rfc2822 rfc2822 marked this pull request as ready for review July 7, 2025 16:29
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR extracts the event build and populate logic out of AndroidEvent into two focused mapping classes, improving separation of concerns without altering existing behavior.

  • Introduce AndroidEventProcessor for reading Android provider rows into Event.
  • Introduce AndroidEventBuilder for writing an Event back to Android provider rows.
  • Refactor AndroidEvent to delegate to these new classes and remove inline mapping code; update package and imports in AttendeeMappings.

Reviewed Changes

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

Show a summary per file
File Description
AttendeeMappings.kt Updated package, docs, and simplified getParameter calls.
AndroidEventProcessor.kt Added new class handling populate logic from provider rows.
AndroidEventBuilder.kt Added new class handling build logic into provider rows.
AndroidEvent.kt Refactored to delegate to builder/processor classes, removed inline mapping code.
AttendeeMappingsTest.kt Updated package declaration and reformatted assertions.
AndroidEventProcessorTest.kt Moved tests to new mapping package; no logic changes.
Comments suppressed due to low confidence (3)

lib/src/main/kotlin/at/bitfire/synctools/mapping/calendar/AndroidEventBuilder.kt:1

  • [nitpick] There are thorough tests for AndroidEventProcessor, but no dedicated unit tests for AndroidEventBuilder. Adding coverage for builder code paths—especially around recurrence rules, exceptions, and extended properties—would help ensure correctness during future refactors.
/*

lib/src/main/kotlin/at/bitfire/synctools/mapping/calendar/AttendeeMappings.kt:20

  • [nitpick] The reference [Attendees] can be ambiguous in generated docs. Consider using a fully qualified link, e.g. [CalendarContract.Attendees], or importing it explicitly so readers know exactly which API is being mapped.
 * Defines mappings between Android [Attendees] and iCalendar parameters.

lib/src/main/kotlin/at/bitfire/synctools/mapping/calendar/AndroidEventBuilder.kt:304

  • This mutates the original event.rDates list, which may cause unexpected side effects if the same Event instance is reused. Consider operating on a copy of the recurrence list or using a local temporary list before building the Android row.
                    event.rDates.addFirst(RDate(listWithDtStart))

@rfc2822 rfc2822 requested a review from sunkup July 7, 2025 16:32
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Why are 12 tests failing when there has been no logic changes, but only rearrangements and slight additions? Since you requested the review with failing tests, will the tests be fixed in a new PR then? I think we should keep everything in a working state. Or is that not possible?

@github-project-automation github-project-automation bot moved this from Todo to In Progress in DAVx⁵ Releases Jul 8, 2025
@rfc2822 rfc2822 marked this pull request as draft July 8, 2025 09:12
@rfc2822 rfc2822 force-pushed the introduce-mapping branch from be2b19d to ce46844 Compare July 8, 2025 14:29
@rfc2822
Copy link
Member Author

rfc2822 commented Jul 8, 2025

Why are 12 tests failing when there has been no logic changes, but only rearrangements and slight additions?

Looks like it was caused by the shared time zone registry in DateUtils. Should be fixed by #36 (which is already merged into this PR).

@rfc2822 rfc2822 marked this pull request as ready for review July 8, 2025 15:11
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did the force push remove the previous commits from ths PR? The top comment talks about changes that are not present (anymore)? Or have those changes been merged into the main branch already?

@github-actions
Copy link

github-actions bot commented Jul 9, 2025

This PR/issue depends on:

1 similar comment
@github-actions
Copy link

github-actions bot commented Jul 9, 2025

This PR/issue depends on:

@rfc2822 rfc2822 marked this pull request as draft July 9, 2025 10:02
rfc2822 added 2 commits July 9, 2025 12:04
Merge branch 'main' into introduce-mapping

# Conflicts:
#	lib/src/androidTest/kotlin/at/bitfire/ical4android/AndroidEventTest.kt
#	lib/src/main/kotlin/at/bitfire/ical4android/AndroidEvent.kt
@rfc2822 rfc2822 force-pushed the introduce-mapping branch from 5c5939e to c90aa10 Compare July 9, 2025 10:12
@rfc2822 rfc2822 marked this pull request as ready for review July 9, 2025 10:23
@rfc2822 rfc2822 requested a review from sunkup July 9, 2025 10:23
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok

@rfc2822 rfc2822 merged commit a9ac952 into main Jul 9, 2025
10 checks passed
@rfc2822 rfc2822 deleted the introduce-mapping branch July 9, 2025 11:02
@github-project-automation github-project-automation bot moved this from In Progress to Done in DAVx⁵ Releases Jul 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring Quality improvement of existing functions

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants