Skip to content

Introduce tasks mapping#174

Merged
sunkup merged 5 commits intomainfrom
introduce-tasks-mapping
Jan 29, 2026
Merged

Introduce tasks mapping#174
sunkup merged 5 commits intomainfrom
introduce-tasks-mapping

Conversation

@sunkup
Copy link
Copy Markdown
Member

@sunkup sunkup commented Jan 28, 2026

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

  • Move builder/processor logic to separate classes.
  • Move tests (only testBuild... methods, since populate... methods don't have tests) - meaning there was no need to create a file DmfsTaskProcessorTest
  • Adapt some tests to account for the changes
  • Split test testGetTimeZone into three tests

The interesting part happens in DmfsTask (especially add() and update()). The logic of building/populating has not been touched. I, however, saw it fit to add a comment indicating that one needs to "get nextBackrefIdx BEFORE adding builder to batch") as it broke the logic for me when I simplified the code by changing the order.

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

@sunkup sunkup self-assigned this Jan 28, 2026
@sunkup sunkup added the refactoring Quality improvement of existing functions label Jan 28, 2026
@sunkup sunkup marked this pull request as ready for review January 28, 2026 13:51
@sunkup sunkup requested a review from a team as a code owner January 28, 2026 13:51
@sunkup sunkup requested a review from ArnyminerZ January 28, 2026 13:51
Copy link
Copy Markdown
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Didn't look through the details, but looks good to me in principle.

Feel free to merge if you have tested everything. As soon as the corresponding DAVx5 PR is merged and works, I'll create an alpha/beta version so that we have additional testing.

@rfc2822 rfc2822 removed the request for review from ArnyminerZ January 28, 2026 15:25
@sunkup
Copy link
Copy Markdown
Member Author

sunkup commented Jan 29, 2026

As soon as the corresponding DAVx5 PR is merged and works, I'll create an alpha/beta version so that we have additional testing.

Which would that be? The similar PR (calendar): #32 - which this one is based on - does not have a matching DAVx⁵ PR. Or do you just mean the next relevant PR in DAVx⁵ ?

The next one (calendar) in DAVx⁵ would be bitfireAT/davx5-ose#1573 but that one goes with #41

@sunkup sunkup merged commit 36982dc into main Jan 29, 2026
7 checks passed
@sunkup sunkup deleted the introduce-tasks-mapping branch January 29, 2026 10:16
@rfc2822
Copy link
Copy Markdown
Member

rfc2822 commented Jan 29, 2026

Which would that be? The similar PR: #32 - which this one is based on - does not have a matching DAVx⁵ PR. Or do you just mean the next relevant PR in DAVx⁵ ?

Then please one that only updates synctools. I'd like to have the changes in the alpha/beta

@sunkup sunkup mentioned this pull request Jan 29, 2026
4 tasks
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

None yet

Development

Successfully merging this pull request may close these issues.

2 participants