Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move local collection management from companion objects to LocalDataStore #1125

Merged
merged 13 commits into from
Nov 18, 2024

Conversation

rfc2822
Copy link
Member

@rfc2822 rfc2822 commented Nov 9, 2024

Purpose

Refactors the code by relocating the management of local collections from companion objects to a dedicated LocalDataStore interface + implementations.

Goal is to explicitly define who is responsible for managing (CRUD) local collections (address books, calendars etc). Usage of DI allows to fetch settings in the specific implementations, which makes calling classes like the Syncer sub-classes much shorter.

TODO:

  • simplify repeated Collection → Service → Account query
  • tests

Short description

  • Define a new LocalDataStore interface, which is responsible for managing local collections (= the ones accessed over a content provider).
  • Implementations for address books, calendars, jtx Board collections, tasks.org task lists.
  • Move as much unstructured code from companion objects to a location where it belongs to.

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

- note that things like the "sync_enabled" Android calendar flag are not supported and always set to true
@rfc2822 rfc2822 self-assigned this Nov 9, 2024
@rfc2822 rfc2822 added the refactoring Internal improvement of existing functions label Nov 9, 2024
@rfc2822 rfc2822 changed the title Localdatastore Move local collection management from companion objects to LocalDataStore Nov 9, 2024
@rfc2822
Copy link
Member Author

rfc2822 commented Nov 11, 2024

@sunkup Can you please have a look and adapt the tests? (SyncerTest etc.)

@sunkup sunkup marked this pull request as ready for review November 12, 2024 13:25
@sunkup sunkup self-requested a review November 12, 2024 13:25
sunkup
sunkup previously approved these changes Nov 12, 2024
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.

@rfc2822 I have fixed the tests using mockk injection - which is really cool I think. There is some small stuff left, but it looks really good to me otherwise and sync seems to work just fine.

@sunkup sunkup marked this pull request as draft November 12, 2024 13:57
@rfc2822
Copy link
Member Author

rfc2822 commented Nov 12, 2024

Incorporated the suggestions.

I wonder how/whether we could test the stores as well. If you have ideas, please go ahead @sunkup

@sunkup
Copy link
Member

sunkup commented Nov 14, 2024

Incorporated the suggestions.

I wonder how/whether we could test the stores as well. If you have ideas, please go ahead @sunkup

I will mockk around some more ;)

@rfc2822 rfc2822 marked this pull request as ready for review November 18, 2024 11:11
@rfc2822 rfc2822 marked this pull request as draft November 18, 2024 11:12
@sunkup sunkup marked this pull request as ready for review November 18, 2024 11:44
@rfc2822 rfc2822 merged commit 0f4e48a into main-ose Nov 18, 2024
8 checks passed
@rfc2822 rfc2822 deleted the localdatastore branch November 18, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Internal improvement of existing functions
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants