pocket-casts-android icon indicating copy to clipboard operation
pocket-casts-android copied to clipboard

Published 20 hours ago • verified publisher icon Automattic

Add type-safe Tracks properties

Open mchowning opened this issue 1 year ago • 0 comments

I'm keeping this as a draft PR, but please go ahead and take a look at it.

Description

This PR is more of me thinking out loud than anything else. I got started down this path because on more than one occassion I have sent an eventprop to Tracks that was not a valid prop (frequently I would sent the object instead of extracting out the String/Boolean/number I needed) and, as a result, it would show up in Tracks as null. I got to thinking about how we could add some type safety around that, and wanted to play with the idea of creating a wrapper class that could only be constructed with a "valid" type, so instead of how we're now using Map<String, Any> we would be using Map<String, AnalyticsPropValue> for our event properties.

Note This PR has a lot of changes, but the critical changes are in the AnalyticsPropValue.kt and AnalyticsTracker.kt files. The majority of the other changes are just fixing the type errors resulting from the changes in those files.

I like how this makes passing the wrong type a compile-time error and how we're now passing around the AnalyticsPropValue type instead of bunch of Strings, numbers, and Booleans. With that said, this adds some boilderplate to the codebase, so I'm not completely sure it's worth it.

You'll notice that I also made a few other changes as I was looking through the files, but those are not central to this and I'm more just curious to hear your thoughts since I think a lot of them fall into the personal preference realm. I'm happy to rewrite this PR with only the changes we want (if we even want any of the changes).

Checklist

  • [X] Should this change be included in the release notes? If yes, please add a line in CHANGELOG.md
  • [X] Have you tested in landscape?
  • [X] Have you tested accessibility with TalkBack?
  • [X] Have you tested in different themes?
  • [X] Does the change work with a large display font?
  • [X] Are all the strings localized?
  • [X] Could you have written any new tests?
  • [X] Did you include Compose previews with any components?

mchowning avatar Oct 10 '22 16:10 mchowning