Conversation
df3c10c to
28e7369
Compare
lib/src/androidTest/kotlin/at/bitfire/synctools/storage/tasks/DmfsTaskListTest.kt
Outdated
Show resolved
Hide resolved
rfc2822
left a comment
There was a problem hiding this comment.
I think there are unoptimized imports. Looks good otherwise on the first glance!
6adf714 to
4f64f5e
Compare
4f64f5e to
477fba0
Compare
|
Ok. Ready for review. Please pay close attention to the uri helper methods. That is: "tasksUri", "taskListUri", etc. I found those a tad confusing ... |
rfc2822
left a comment
There was a problem hiding this comment.
Exception handling: sometimes we have a try/catch Exception (with re-throw to LocalStorageException), sometimes try/catch RemoteException, and sometimes no try/catch.
I think synctools should make sure that every call throws a LocalStorageException in case of errors, but not a RemoteException. So I'd catch RemoteException on all provider operations and use it as the cause of the LocalStorageException.
Generally, how do you want to handle tests? Always have almost-complete tests in every PR or do you want to migrate the architecture first and then, as soon as a class is "as it should be", make sure that all its tests are written?
lib/src/main/kotlin/at/bitfire/synctools/storage/tasks/DmfsTaskListProvider.kt
Outdated
Show resolved
Hide resolved
lib/src/androidTest/kotlin/at/bitfire/synctools/storage/tasks/DmfsTaskListTest.kt
Outdated
Show resolved
Hide resolved
I would usually want to go by the first approach, but in this case, since I feel there is a certain pressure to be done soon with the rewrite, I'd add the tests when all else is done. |
rfc2822
left a comment
There was a problem hiding this comment.
Didn't check everything in detail but looks good!
Part of refactoring synctools tasks.
Required for bitfireAT/davx5-ose#1934 .
For reference only: Similar AndroidCalendar changes are made in #28