Migrate `ParselyTracker` from Java to Kotlin #99

wzieba · 2023-12-13T17:32:35Z

Best review after #100

Description

This PR migrates ParselyTracker, from Java to Kotlin. This class is the starting point for any user of this SDK.

Aside from the migration, this PR introduces also default arguments for tracking methods, to make calling this SDK from Kotlin more convenient, some code simplifications. This PR also introduces enforcing visibility modifiers for the whole library by setting explicitApi to STRICT.

Testing

This PR does not require testing. Functional tests should assert the validity of basic flows.

To fix functional tests execution

…ton getter

…ads` annotation

The `sharedInstance` that creates a new instance of `ParselyTracker` should not have nullable return type

To improve API usage for both Kotlin and Java SDK consumers

… methods/fields

…ely_tracker_kotlin_migration

Due to Java->Kotlin conversion, this condition was lost during merge.

…viewUuid` I think there's no a lot of value in providing `null` in `extraData` items

Fix missing visibility modifiers

codecov · 2023-12-13T18:19:04Z

Codecov Report

Attention: 90 lines in your changes are missing coverage. Please review.

Comparison is base (2c64338) 71.81% compared to head (a8bf91d) 68.75%.
Report is 1 commits behind head on main.

❗ Current head a8bf91d differs from pull request most recent head c2cb617. Consider uploading reports for the commit c2cb617 to get more accurate results

Files	Patch %	Lines
.../java/com/parsely/parselyandroid/ParselyTracker.kt	24.36%	90 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #99      +/-   ##
==========================================
- Coverage   71.81%   68.75%   -3.07%     
==========================================
  Files          18       18              
  Lines         369      400      +31     
  Branches       52       49       -3     
==========================================
+ Hits          265      275      +10     
- Misses         92      113      +21     
  Partials       12       12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wzieba · 2023-12-13T19:29:55Z

parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.kt

+ * Accessed as a singleton. Maintains a queue of pageview events in memory and periodically
+ * flushes the queue to the Parse.ly pixel proxy server.
+ */
+public open class ParselyTracker protected constructor(siteId: String, flushInterval: Int, c: Context) {


This shouldn't be public and open but I haven't found any other way for now. If I introduced some interface, let's say Tracker to use it in EngagementManagerTest to create a fake implementation, I'd have to expose enqueueEvent to public - apparently, interface methods cannot be internal.

But I believe this shouldn't be a blocker - ParselyTracker, as previously written in Java, was "open" by default.

If I introduced some interface, let's say Tracker to use it in EngagementManagerTest to create a fake implementation, I'd have to expose enqueueEvent to public - apparently, interface methods cannot be internal.

Assuming that this is what we wanted to do, wouldn't it be more appropriate to use multiple interfaces in this case? We'd have a Tracker interface that we expose to public and another internal interface that has enqueueEvent function. ParselyTracker then will implement both and everything should work? Let me know if I am missing anything in this scenario.

Even with multiple interfaces, we can't have a method in interface that is internal. Even if the interface itself is internal

If we didn't add internal, then ParselyTracker#enqueueEvent would be exposed to API consumers, which is something we should omit.

It's very likely that I am missing something, but I was thinking something like this would work? 74a9f18

Note that this ^ is not production quality code, I just put it together to make it easier to discuss. Let me know what you think!

Also, if we did go with such a design, I think we can make ParselyTracker internal as well and expose the shared instance some other way.

Thank you! I really like your idea, haven't seen in this way myself. I also appreciate it clarifies the public contract better.

I've cherry-picked your comment and added some changes. Most notably, I've renamed Tracker interface to ParselyTracker to minimize breaking changes for the API consumers.

I am glad you found it useful! 🥳

…ely_tracker_kotlin_migration

oguzkocer

@wzieba I finished my first pass of this, but I might have more to add in my second pass as I want to think about it a bit. Hope you find the comments useful!

parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.kt

oguzkocer · 2024-01-10T18:51:32Z

parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.kt

+ * Accessed as a singleton. Maintains a queue of pageview events in memory and periodically
+ * flushes the queue to the Parse.ly pixel proxy server.
+ */
+public open class ParselyTracker protected constructor(siteId: String, flushInterval: Int, c: Context) {


If I introduced some interface, let's say Tracker to use it in EngagementManagerTest to create a fake implementation, I'd have to expose enqueueEvent to public - apparently, interface methods cannot be internal.

Assuming that this is what we wanted to do, wouldn't it be more appropriate to use multiple interfaces in this case? We'd have a Tracker interface that we expose to public and another internal interface that has enqueueEvent function. ParselyTracker then will implement both and everything should work? Let me know if I am missing anything in this scenario.

BREAKING CHANGE: This commit removes `setDebug` method which is a part of public contract. Instead, API consumers should set debug parameter when invoking `sharedInstance` method.

I believe it better communicates the intention than `debug` flag.

We either way were setting empty string in case of nullable `urlRef`. Now it's more explicit. BREAKING CHANGE: As `urlRef` values are no longer nullable, this breaks the public contract.

For better Java co-op.

wzieba · 2024-01-11T12:51:03Z

Thank you @oguzkocer for the review! Ready for the second round.

Reason: to minimize breaking changes for the clients

wzieba · 2024-01-15T14:58:09Z

Thank you @oguzkocer for the suggestions! Applied them, ready for another round.

oguzkocer

@wzieba Thank you for addressing my previous feedback. I am glad that it was useful.

I had a one last look and wanted to bring up a couple points about the singleton. I am approving the changes just so this PR doesn't block you anymore, but I am curious to hear your thoughts on those.

In general, it's looking really good and the SDK is looking significantly better 🙇‍♂️

oguzkocer · 2024-01-16T17:20:09Z

parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.kt

+        /**
+         * Singleton instance accessor. Note: This must be called after [.sharedInstance]
+         *
+         * @return The singleton instance
+         */
+        @JvmStatic
+        public fun sharedInstance(): ParselyTracker? {
+            return instance
+        }


I am not sure if the additional sharedInstance method is helpful or not. I think the following is the intended usage:

First call sharedInstance({configuration})

Subsequently call sharedInstance()

Although this works - the better approach in my opinion is to:

First call sharedInstance({configuration}) and assign it to a property (preferably somewhere where you can inject it)

Then only access the tracker through your local (to the client binary) property and not use the sharedInstance()

This approach is better in my opinion, because it doesn't deal with the nullability and somewhat forces the instantiation of the property to be done in a more proper place. It's also how a client would handle the tracker if it wasn't using a singleton pattern.

So, I wonder if we should not give them the option to use the first approach at all. What do you think?

oguzkocer · 2024-01-16T17:27:47Z

parsely/src/main/java/com/parsely/parselyandroid/ParselyTracker.kt

+        @JvmStatic
+        @JvmOverloads
+        public fun sharedInstance(
+            siteId: String,
+            flushInterval: Int = DEFAULT_FLUSH_INTERVAL_SECS,
+            context: Context,
+            dryRun: Boolean = false,
+        ): ParselyTracker {


A singleton that requires a configuration presents an interesting problem. What happens if the client tries to pass a different configuration after the instance is already initialized?

Currently, the secondary configuration is silently ignored. And, I could see a case where this can result in developers trying to figure out why their configuration change is not applied. Assuming that we don't want to allow updating the configuration after the initialization, I think we can give them a heads up about this in the documentation and by printing a log.

How do you feel about:

Adding a note to the documentation of this method, saying that this method should only be called once and subsequent calls will ignore its arguments and return the existing instance

Either add a generic warning about this ^ as a log every time we return the existing instance. Or - add a specific warning if this method is called with a different configuration as compared to the existing instance.

Note that an alternative approach that would avoid this whole issue would be to use a build time configuration instead of function parameters, but I don't think that's appropriate for this use case. Even if it was, that approach has its own issues - but I wanted to mention it for completeness-sake.

wzieba · 2024-01-17T18:07:00Z

Thank you @oguzkocer for the comments about ParselyTracker#sharedInstance! I think it'll be the best to continue this discussion in a separate PR - merging this one, prepare changes in another PR and ping you there 🙂.

wzieba added 16 commits December 13, 2023 16:53

Rename .java to .kt

f1dedc6

refactor: move ParselyTracker to Kotlin

2e39558

fix: signature of ParselyTracker#enqueueEvent

119deb6

fix: make ROOT_URL a @JvmField

ead2012

To fix functional tests execution

style: simplify tracking methods

c782128

style: simplify ParselyTracker methods and fix minor code style issues

a583f37

fix: add JvmStatic annotation to expect-to-be static in Java single…

b08df47

…ton getter

fix: siteId must be not null

0a8a010

refactor: remove on of shareInstance method in favor of `@JvmOverlo…

7b02055

…ads` annotation

refactor: sharedInstance might return non-nullable instance

857127a

The `sharedInstance` that creates a new instance of `ParselyTracker` should not have nullable return type

feat: add default null values and @JvmOverloads to tracking methods

cad7ca2

To improve API usage for both Kotlin and Java SDK consumers

feat: enable explicitMode and add visibility modifiers to all exposed…

568b3ee

… methods/fields

Merge branch 'issue/migrate_events_builder_to_kotlin' into issue/pars…

aec2bb2

…ely_tracker_kotlin_migration

fix: apply missing condition after merge

a9bca1c

Due to Java->Kotlin conversion, this condition was lost during merge.

fix: update signature of extraData and fix nullability of `lastPage…

ea2e69f

…viewUuid` I think there's no a lot of value in providing `null` in `extraData` items

build: set explicitApi to strict mode

2e77807

Fix missing visibility modifiers

wzieba changed the base branch from main to issue/migrate_events_builder_to_kotlin December 13, 2023 18:18

wzieba added enhancement do not merge labels Dec 13, 2023

wzieba changed the title ~~[WIP] Migrate ParselyTracker from Java to Kotlin~~ Migrate ParselyTracker from Java to Kotlin Dec 13, 2023

wzieba marked this pull request as ready for review December 13, 2023 19:23

wzieba requested a review from oguzkocer December 13, 2023 19:23

wzieba commented Dec 13, 2023

View reviewed changes

wzieba added 3 commits December 14, 2023 13:06

Merge branch 'issue/migrate_events_builder_to_kotlin' into issue/pars…

8e23ea1

…ely_tracker_kotlin_migration

refactor: simplify ParselyTracker#trackPlay method

4a9c0bd

refactor: simplify ParselyTracker methods post Kotlin migration

42325ed

oguzkocer reviewed Jan 10, 2024

View reviewed changes

Base automatically changed from issue/migrate_events_builder_to_kotlin to main January 11, 2024 09:51

feat: set debug flag in static sharedInstance

27fab68

BREAKING CHANGE: This commit removes `setDebug` method which is a part of public contract. Instead, API consumers should set debug parameter when invoking `sharedInstance` method.

wzieba added 5 commits January 11, 2024 12:43

refactor: rename debug to dryRun

9d75f60

I believe it better communicates the intention than `debug` flag.

fix: make urlRef not nullable

ea37072

We either way were setting empty string in case of nullable `urlRef`. Now it's more explicit. BREAKING CHANGE: As `urlRef` values are no longer nullable, this breaks the public contract.

feat: add @JvmOverloads to trackPageview

cb4b376

For better Java co-op.

tests: add dryRun argument for FakeTracker

39570aa

feat: log when engagement session is stopped

a8bf91d

oguzkocer and others added 7 commits January 15, 2024 14:34

Introduce Tracker & EventQueuer interfaces

06aab22

refactor: move sharedInstance methods to the interface

f02483e

refactor: rename interface Tracker to ParselyTracker

f0020c0

Reason: to minimize breaking changes for the clients

tests: fix functional tests after rename

63e228c

refactor: move interfaces to separate files

cfabce7

fix: make urlRef not nullable in startEngagement

697c051

fix: visibility modifiers for ParselyTrackerInternal companion object

c2cb617

wzieba requested a review from oguzkocer January 15, 2024 14:58

wzieba removed the do not merge label Jan 15, 2024

oguzkocer approved these changes Jan 16, 2024

View reviewed changes

wzieba merged commit d4e225d into main Jan 17, 2024
2 checks passed

wzieba deleted the issue/parsely_tracker_kotlin_migration branch January 17, 2024 18:07

wzieba mentioned this pull request Jan 18, 2024

Improve how ParselyTracker is accessed #105

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `ParselyTracker` from Java to Kotlin #99

Migrate `ParselyTracker` from Java to Kotlin #99

wzieba commented Dec 13, 2023 •

edited

codecov bot commented Dec 13, 2023 •

edited

wzieba Dec 13, 2023

oguzkocer Jan 10, 2024

wzieba Jan 11, 2024

oguzkocer Jan 11, 2024

wzieba Jan 15, 2024

oguzkocer Jan 16, 2024

oguzkocer left a comment

oguzkocer Jan 10, 2024

wzieba commented Jan 11, 2024

wzieba commented Jan 15, 2024

oguzkocer left a comment

oguzkocer Jan 16, 2024

oguzkocer Jan 16, 2024

wzieba commented Jan 17, 2024

Migrate ParselyTracker from Java to Kotlin #99

Migrate ParselyTracker from Java to Kotlin #99

Conversation

wzieba commented Dec 13, 2023 • edited

Description

Testing

codecov bot commented Dec 13, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oguzkocer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzieba commented Jan 11, 2024

wzieba commented Jan 15, 2024

oguzkocer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wzieba commented Jan 17, 2024

Migrate `ParselyTracker` from Java to Kotlin #99

Migrate `ParselyTracker` from Java to Kotlin #99

wzieba commented Dec 13, 2023 •

edited

codecov bot commented Dec 13, 2023 •

edited