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

[WIP] Store 5: Multiplatform Write Support #443

Closed
wants to merge 2 commits into from

Conversation

matt-ramotar
Copy link
Collaborator

Closes #247, #50, #36, #35, #25

@matt-ramotar matt-ramotar changed the title [WIP] Network-resilient repository layer for Android and iOS [WIP] Add Multiplatform Write Support Sep 6, 2022
@matt-ramotar matt-ramotar added the enhancement New feature or request label Sep 6, 2022
@matt-ramotar matt-ramotar changed the title [WIP] Add Multiplatform Write Support [WIP] Store 5: Multiplatform Write Support Sep 6, 2022
@@ -18,6 +18,9 @@ buildscript {
classpath "org.jacoco:org.jacoco.core:${versions.jacocoGradlePlugin}"
classpath "org.jetbrains.kotlinx:atomicfu-gradle-plugin:${versions.atomicFuPlugin}"
classpath "com.vanniktech:gradle-maven-publish-plugin:${versions.mavenPublishPlugin}"
classpath("app.cash.zipline:zipline-gradle-plugin:0.9.1")
classpath("com.squareup.sqldelight:gradle-plugin:1.5.3")
Copy link
Contributor

Choose a reason for hiding this comment

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

move versions to dependencies.gradle

@@ -18,6 +18,9 @@ buildscript {
classpath "org.jacoco:org.jacoco.core:${versions.jacocoGradlePlugin}"
classpath "org.jetbrains.kotlinx:atomicfu-gradle-plugin:${versions.atomicFuPlugin}"
classpath "com.vanniktech:gradle-maven-publish-plugin:${versions.mavenPublishPlugin}"
classpath("app.cash.zipline:zipline-gradle-plugin:0.9.1")
classpath("com.squareup.sqldelight:gradle-plugin:1.5.3")
classpath("com.google.dagger:hilt-android-gradle-plugin:2.42")
Copy link
Contributor

Choose a reason for hiding this comment

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

in this house we use anvil :-D jk, glad you got this into the sample, extract version pls

@@ -26,7 +29,7 @@ apply from: 'buildsystem/dependencies.gradle'
apply plugin: 'binary-compatibility-validator'

apiValidation {
ignoredProjects += ["app"]
ignoredProjects += ["reddit", "store", "samples", "notes", "android", "common", "fig"]
Copy link
Contributor

Choose a reason for hiding this comment

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

is this temporary? You can run gradlew apiDump to regen the api check file

@@ -0,0 +1,7 @@
plugins {
`kotlin-dsl`
Copy link
Contributor

Choose a reason for hiding this comment

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

why you don't like groovy?

const val store = "com.dropbox.mobile.store:store4:4.0.5"
}

object Koin {
Copy link
Contributor

Choose a reason for hiding this comment

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

why koin?

@@ -14,7 +14,7 @@ dependencies {
implementation libraries.okio
implementation libraries.coroutinesCore
implementation libraries.cache
implementation project(path: ':store')
implementation project(path: ':store4')
Copy link
Contributor

Choose a reason for hiding this comment

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

change back, we keep module as store but artifacts as store5, same for the package

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey I don't follow - My understanding was current :store becomes :store4. Then Store 5 becomes new :store. And :filesystem, :store-rx2, and :store-rx3 depend on :store4 rather than :store. What am I missing?

Copy link
Contributor

@digitalbuddha digitalbuddha Sep 6, 2022

Choose a reason for hiding this comment

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

https://github.com/MobileNativeFoundation/Store/tree/main/store/src/main/java/com/dropbox/android/external/store4 rename this package to store5. Rename the maven artifacts to store5. We don't need to have store4 and store5 in repo at same time. Your change can be a breaking change. The important thing is someone can add the store4 and store5 artifacts at same time. There should be 0 references to store4 once all your changes land.

import io.ktor.http.HttpStatusCode
import kotlin.collections.set

class FakeApi(private val client: HttpClient) : Api<Key, Notebook> {

Choose a reason for hiding this comment

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

You can move this and the other Fake implementations out of the main source path and into the relevant test path

}

override suspend fun post(note: MarketNote): Boolean {
return try {

Choose a reason for hiding this comment

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

nit: can make these functions more expressive by removing their brackets and return statements.

@digitalbuddha
Copy link
Contributor

Discussed offline to split this between 3 diffs

  • store5 rename
  • breaking store changes
  • new sample

@matt-ramotar
Copy link
Collaborator Author

Update Mike and I synced earlier today - I will work off store5 branch. Our plan is to put up smaller PRs and merge with main at end. I created a milestone and scoped out main issues here

@matt-ramotar
Copy link
Collaborator Author

Thanks @digitalbuddha @theBradfo for early feedback. Will incorporate suggestions above in upcoming diffs

@matt-ramotar matt-ramotar deleted the matt-ramotar/market branch February 19, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
3 participants