-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Make managing project dependencies more consistent #6158
Merged
Merged
Conversation
This file contains 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
seadowg
force-pushed
the
dependency-provider
branch
7 times, most recently
from
June 3, 2024 12:59
928c96d
to
f2b977b
Compare
I think it'd also be good to introduce a new convenience abstraction for building data services here. Something like: abstract class DataService(private val appState: AppState) {
fun <T> getData(key: String, default: T): Data<T> {
return Data(appState, key, default)
}
}
class Data<T>(private val appState: AppState, private val key: String, private val default: T) {
fun get(qualifier: String? = null): Flow<T> {
return appState.getFlow("$qualifier:$key", default)
}
fun set(value: T, qualifier: String? = null) {
appState.setFlow("$qualifier:$key", value)
}
} |
6 tasks
seadowg
force-pushed
the
dependency-provider
branch
from
June 11, 2024 17:04
f2b977b
to
61a1f00
Compare
seadowg
force-pushed
the
dependency-provider
branch
2 times, most recently
from
June 24, 2024 15:33
f02e968
to
e82ded5
Compare
@grzesiek2010 we probably want to wait on merging this until we've got the last release things in, but I'd like to get your initial thoughts. |
seadowg
force-pushed
the
dependency-provider
branch
from
July 12, 2024 09:25
d8c9c13
to
91be0f1
Compare
grzesiek2010
requested changes
Jul 15, 2024
collect_app/src/main/java/org/odk/collect/android/activities/DeleteFormsActivity.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/formmanagement/FormsDataService.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/instancemanagement/InstancesDataService.kt
Outdated
Show resolved
Hide resolved
...ect_app/src/test/java/org/odk/collect/android/instancemanagement/InstancesDataServiceTest.kt
Outdated
Show resolved
Hide resolved
...p/src/test/java/org/odk/collect/android/application/initialization/SavepointsImporterTest.kt
Outdated
Show resolved
Hide resolved
collect_app/src/main/java/org/odk/collect/android/injection/config/AppDependencyComponent.java
Outdated
Show resolved
Hide resolved
...ect_app/src/test/java/org/odk/collect/android/instancemanagement/InstancesDataServiceTest.kt
Outdated
Show resolved
Hide resolved
grzesiek2010
requested changes
Jul 15, 2024
...p/src/test/java/org/odk/collect/android/application/initialization/SavepointsImporterTest.kt
Outdated
Show resolved
Hide resolved
seadowg
force-pushed
the
dependency-provider
branch
from
July 15, 2024 15:07
167e9bd
to
b477e21
Compare
grzesiek2010
approved these changes
Jul 15, 2024
Tested with Success! Verified on a device with Android 10 Verified Cases:
|
Tested with Success! Verified on a device with Android 14 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The idea here is to create a consistent way to create project level dependencies in objects that interact with multiple projects (like data services) to make it easier to write tests and to create ways of providing those dependencies.
This approach isn't completely finished. I've established the pattern, but some of our "providers" and their usages (like
StoragePathProvider
) would need significant rework and the main thing I wanted to get to here was a route forward. In those cases I've deprecated previous ways of access dependencies so we can remove them gradually.I've also added some helpers for data services so we can keep the implementation more consistent in the form of new extensions for
AppState
.Why is this the best possible solution? Were any other approaches considered?
I did initially look at using Dagger's "assisted injection", but I found this wasn't ideal. For instance, I'd end up with factories like this:
There are two problems here:
DatabaseFormsRepository
needs to have a constructor that knows about projects which we're trying to avoid.Given all that, I settled for a similar concept (injecting factories with a standard pattern for dependencies that need runtime info), but just hand make the factories instead of relying on Dagger injection.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Do we need any specific form for testing your changes? If so, please attach one.
There's been a bunch of changes across the code base, but I think the main thing to check is that forms, instances and other pieces of data show up in the correct project.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest
(or./gradlew testLab
) and confirmed all checks still passDateFormatsTest