Refactor to support JVM #13

prayansh · 2021-06-24T21:42:55Z

Kind of a big refactor, so will try to capture as much context as possible.
We have refactored our analytics-kotlin module into 2 seperate modules in order to support JVM as a first-class source. The 2 components

core: main analytics module
- Analytics functions
- Platform / Plugin architecture
- Plugins
- Utility Functions
android: android analytics module which defines android-specific functionality, built on top of core
- Analytics constructor
- JSON functions
- Storage
- Plugins

This refactor will allow users to import the analytics-kotlin based on the app type.
If using a JVM kotlin app you would do

dependencies {
    implementation 'com.github.segmentio.kotlin-analytics:core'
}

and for android kotlin app you would do

dependencies {
    implementation 'com.github.segmentio.kotlin-analytics:android'
}

Closes #11

migs647 · 2021-06-24T21:44:49Z

kotlin-android/src/main/java/com/segment/analytics/kotlin/android/utilities/AndroidKVS.kt

+import android.content.SharedPreferences
+import com.segment.analytics.kotlin.core.utilities.KVS
+
+class AndroidKVS(val sharedPreferences: SharedPreferences): KVS {


No description provided.

migs647 · 2021-06-24T21:45:57Z

kotlin-android/src/main/java/com/segment/analytics/kotlin/android/utilities/AndroidKVS.kt

+import com.segment.analytics.kotlin.core.utilities.KVS
+
+class AndroidKVS(val sharedPreferences: SharedPreferences): KVS {
+    override fun getInt(key: String, defaultVal: Int): Int =


Should we consider a default parameter value for defaultVal or add an additional method if they don't want to pass a defaultVal?

I noticed the rest of them were defaulted to 0

this is an internal api, only being used in 1 place currently. im not sure if having default values will give us any additional benefits

migs647 · 2021-06-24T21:47:59Z

kotlin-android/src/test/java/com/segment/analytics/kotlin/android/EventsFileTest.kt

@@ -22,7 +22,8 @@ import java.util.*
 @TestInstance(TestInstance.Lifecycle.PER_CLASS)
 class EventsFileTest {
    private val epochTimestamp = Date(0).toInstant().toString()
-    private val sharedPreferences = MemorySharedPreferences()
+    private val _sharedPreferences = MemorySharedPreferences()
+    private val kvStore = AndroidKVS(_sharedPreferences)


I noticed you used _sharedPreferences for shadow var. We often do this to signify to "not touch" on the obj-c side. Do we want to apply underscore to kvStore as well?

i think this was a refactoring issue. normally ur suggestion would apply, but this is a test file so im not sure if the concept of shadow var / "do not touch" applies here

migs647 · 2021-06-24T21:49:25Z

kotlin-android/src/test/java/com/segment/analytics/kotlin/android/utils/Mocks.kt

+import sovran.kotlin.Store
+import java.io.File
+
+fun mockAnalytics(): Analytics {


? It may be beneficial to add here for when we have new eng coming and knowing if / who they should use this for testing.

migs647 · 2021-06-24T21:52:20Z

kotlin-core/src/main/java/com/segment/analytics/kotlin/core/SegmentDestination.kt

@@ -19,14 +20,14 @@ import java.util.concurrent.atomic.AtomicInteger
 class SegmentDestination(


may be good to add here since it's such an integral part of our library (not related to PR)

migs647 · 2021-06-24T21:52:57Z

kotlin-core/src/main/java/com/segment/analytics/kotlin/core/SegmentDestination.kt

@@ -108,6 +109,7 @@ class SegmentDestination(
    }

    override fun flush() {
+        println("flush")


Remove println or alternate for analytics?.log()?

migs647 · 2021-06-24T21:53:39Z

kotlin-core/src/main/java/com/segment/analytics/kotlin/core/Storage.kt

+import kotlinx.serialization.json.JsonObject
+import sovran.kotlin.Store
+
+interface Storage {


No description provided.

migs647 · 2021-06-24T21:53:53Z

kotlin-core/src/main/java/com/segment/analytics/kotlin/core/Storage.kt

+    }
+}
+
+interface StorageProvider {


No description provided.

migs647 · 2021-06-24T21:58:55Z

kotlin-core/src/main/java/com/segment/analytics/kotlin/core/platform/Mediator.kt


 // Platform abstraction for managing plugins' execution (of a specific type)
 // All operations are thread safe via the `synchronized` function
-internal class Mediator(internal val plugins: MutableList<Plugin>) {
+class Mediator(internal val plugins: MutableList<Plugin>) {


Love the mediator pattern ++!

Should we give it a more specific name? and since it's public now?

im actually going to move it back to being internal and expose the applyClosure function via Analytics

migs647 · 2021-06-24T21:59:28Z

kotlin-core/src/main/java/com/segment/analytics/kotlin/core/platform/plugins/ContextPlugin.kt

+import kotlinx.serialization.json.buildJsonObject
+import kotlinx.serialization.json.put
+
+class ContextPlugin: Plugin {


migs647 · 2021-06-24T22:01:13Z

kotlin-core/src/main/java/com/segment/analytics/kotlin/core/utilities/PropertiesFile.kt

+import java.io.FileOutputStream
+import java.util.*
+
+class PropertiesFile(private val directory: File, writeKey: String): KVS {


migs647 · 2021-06-24T22:01:26Z

kotlin-core/src/main/java/com/segment/analytics/kotlin/core/utilities/StorageImpl.kt

+import sovran.kotlin.Subscriber
+import java.io.File
+
+class StorageImpl(


migs647 · 2021-06-24T22:03:31Z

samples/kotlin-jvm-app/src/main/kotlin/main.kt

+        val dispatcher = coroutineContext[ContinuationInterceptor] as CoroutineDispatcher
+        val analytics = Analytics("gNHARErhCjBxvBErXOMrTTuwoIlxKkCg") {
+            application = "MainApp"
+//            flushInterval = 0


remove comment?

migs647 · 2021-06-24T22:03:47Z

samples/kotlin-jvm-app/src/main/kotlin/main.kt

+            }
+        )
+        analytics.flush()
+        delay(30 * 1000)


Be good to know why we're delaying

migs647

Super nit picky review

migs647 · 2021-06-24T22:05:11Z

kotlin-core/src/test/kotlin/com/segment/analytics/kotlin/core/utils/Plugins.kt

+import com.segment.analytics.kotlin.core.*
+import com.segment.analytics.kotlin.core.platform.*
+
+class TestRunPlugin(override val name: String = "TestRunPlugin", var closure: (BaseEvent?) -> Unit): EventPlugin {


Is this for tests?

yup, added some docs to outline use-cases

migs647 · 2021-06-29T22:52:50Z

android/src/main/java/com/segment/analytics/kotlin/android/AndroidAnalytics.kt

@@ -62,7 +67,7 @@ public fun Analytics(
 }

 // Logger instance that uses the android `Log` class
-object AndroidLogger: Logger("AndroidLogger") {
+object AndroidLogger : Logger("AndroidLogger") {


Are we adding spaces or was this an accident? I noticed others don't have that space.

i ran a auto-lint on the project after the rebase, so all of the files should conform to the same convention

migs647 reviewed Jun 24, 2021

View reviewed changes

samples/kotlin-jvm-app/src/main/kotlin/main.kt

}

)

analytics.flush()

delay(30 * 1000)

Copy link

Contributor

migs647 Jun 24, 2021

Choose a reason for hiding this comment

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

Be good to know why we're delaying

migs647 approved these changes Jun 24, 2021

View reviewed changes

migs647 reviewed Jun 24, 2021

View reviewed changes

prayansh added 14 commits June 29, 2021 10:41

move around modules, and add core-kotlin module

c99ca2e

move context around, fix storage for jvm

f344e58

add kotlin JVM sample app

55b20cc

rename module

60632f9

rename packages

cae6f75

fix core tests

42c3b16

fix android tests

0f6f2f5

fix example app

2592187

fix example apps

31b5afb

code cleanup

32191e4

fix some docs and rename test to tests

a64b41f

add documentation

74cd085

clarify main.kt

f94daa2

move around some code

336cfb8

prayansh added 2 commits June 29, 2021 10:42

rename modules

3479e3e

fix conflicts after rebase from master and fix wildcard imports

dc59df4

prayansh force-pushed the pray/jvm branch from 1ccaf98 to dc59df4 Compare June 29, 2021 17:56

fix tests

c421794

migs647 reviewed Jun 29, 2021

View reviewed changes

prayansh merged commit 7be234e into main Jun 30, 2021

prayansh deleted the pray/jvm branch June 30, 2021 17:26

		@@ -19,14 +20,14 @@ import java.util.concurrent.atomic.AtomicInteger
		class SegmentDestination(

Refactor to support JVM #13

Refactor to support JVM #13

Uh oh!

Conversation

prayansh commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

migs647 Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

migs647 Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

migs647 Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

migs647 Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

migs647 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

prayansh commented Jun 24, 2021 •

edited

Loading

migs647 Jun 24, 2021 •

edited

Loading

migs647 Jun 24, 2021 •

edited

Loading

migs647 Jun 24, 2021 •

edited

Loading

migs647 Jun 24, 2021 •

edited

Loading