Migrate EventsBuilder to Kotlin

[wzieba]

Description

This PR migrates EventsBuilder to Kotlin to allow the SDK to better manage nullability of types and visibility modifiers.

It also addresses a minor bug: before this PR, the SDK allowed to start engagement session (startEngagement) without tracking a page view first (trackPageview). It should not be possible. Internal discussion: p1696436706596339/1696417656.257339-slack-C0533SEJ82H

Testing

As in other PRs: I believe manual testing with an example app is not necessarily. This SDK is covered with unit tests (especially EventsBuilder class) and it has a few functional tests to verify expected behavior broadly.

[codecov]

Codecov Report

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

Comparison is base (fe141cf) 71.50% compared to head (2c64338) 71.81%.

Files	Patch %	Lines
...ava/com/parsely/parselyandroid/ParselyTracker.java	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
+ Coverage   71.50%   71.81%   +0.30%     
==========================================
  Files          18       18              
  Lines         365      369       +4     
  Branches       53       52       -1     
==========================================
+ Hits          261      265       +4     
- Misses         90       92       +2     
+ Partials       14       12       -2     

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

[oguzkocer]

@wzieba I left a non-blocker comment, but otherwise looks good to me.

[oguzkocer]

This is mostly going to be a nitpick, but I guess it affects the architecture, so I'll leave the decision to you whether it's worth addressing:

---

Taking Clock as a parameter in the constructor doesn't feel correct to me. If we are going to take Clock as a parameter, I think buildEvent is the better place for it, because that's where it's used. However, if we are going to do that, we might as well take now as a parameter and not deal with the Clock ourselves.

If we are going to deal with the Clock internally - and not involve the caller, then I think we can just initiate it ourselves and be the owner of it.

---

I am generally in favor of taking whatever is necessary at the point of its actual usage, which in this case would be taking now as a parameter. However, ParselyTracker doesn't own a Clock, so there is some practical benefit to owning it ourselves - unless there are unit testing considerations.

[oguzkocer]

I just reviewed the unit test changes for this as well and I think taking now as a parameter simplifies that as well. I am curious to hear if you had any reservations to use that approach as opposed to taking it as a constructor argument.

[wzieba]

In my opinion, there are two solutions: either Clock in the constructor or now in the buildEvent. I'm okay with the second as well, but below I share my rationale behind Clock in the constructor.

I am curious to hear if you had any reservations to use that approach as opposed to taking it as a constructor argument.

I've put Clock mentally in the same category as DeviceInfoRepository. We don't execute deviceInfoRepository.collectDeviceInfo() outside EventsBuilder and pass the result in buildEvent in the case of this class, and I treated the Clock the same way.

Also putting Clock as a constructor parameter is that in this design, the code communicates that EventsBuilder needs a Clock for building an event, but it doesn't communicate externally why exactly. I think that's good, as it creates less overhead for the client of taking care of Clock at the time of invoking buildEvent.

WDYT such idea of thinking about Clock in the scope of EventsBuilder?

[oguzkocer]

I've put Clock mentally in the same category as DeviceInfoRepository. We don't execute deviceInfoRepository.collectDeviceInfo() outside EventsBuilder and pass the result in buildEvent in the case of this class, and I treated the Clock the same way.

It's interesting that you mention this, because I actually drafted something about this in the review and then discarded it. I'd have done this outside of the EventsBuilder as well and passed in the data.

Also putting Clock as a constructor parameter is that in this design, the code communicates that EventsBuilder needs a Clock for building an event, but it doesn't communicate externally why exactly. I think that's good, as it creates less overhead for the client of taking care of Clock at the time of invoking buildEvent.

I see this as complete opposite. If possible, I want to know why you need a clock when you are building the event. So for example, if this was in an SDK and I needed to directly use it, as the client I'd ask, why do you need me to pass you a clock? If it's for the event time, why not ask for it. In fact, even at the cost of a bit extra memory, in my opinion the most ergonomic design would be something like:

fun buildEvent(
    url: String,
    urlRef: String,
    action: String,
    metadata: ParselyMetadata?,
    extraData: Map<String, Any>?,
    uuid: String,
    eventTime: Long = Clock().now.inWholeMilliseconds
): Map<String, Any> {

Anyhow - I don't think this is a particularly important one to pick one solution or the other. So, I am happy with whatever you decide to go with :)

[wzieba]

I see this as complete opposite. If possible, I want to know why you need a clock when you are building the event. So for example, if this was in an SDK and I needed to directly use it, as the client I'd ask, why do you need me to pass you a clock?

That's a very interesting perspective! I'd lean more towards asking API: "why do you bother me with passing now or anything that you could figure out before". So that's definitely a refreshing view on the topic to me and food for thoughts 🙂. Thanks for sharing this, I'm glad we exchanged the views!

Anyhow - I don't think this is a particularly important one to pick one solution or the other. So, I am happy with whatever you decide to go with :)

True - it's internal for the API, and we can always iterate without any risk of breaking public contract. I'll default to the currently proposed implementation, just because it doesn't involve more work.

[oguzkocer]

That's a very interesting perspective! I'd lean more towards asking API: "why do you bother me with passing now or anything that you could figure out before".

That's a good point, but it's also applicable to why we are taking Clock as a constructor or function argument as well. It's something we can figure out ourselves. I assume the reason we do it is for testing and I have mixed feelings about that. Generally, I don't love having to change the API just for testing, but I understand that it's sometimes a necessary evil.

I also made my suggestion as eventTime instead of taking now as an argument, because I can see a use case for passing a different event time then the current time.

Anyhow, I am totally fine with keeping this as is, just wanted to clarify a few points.