Migrate ParselyMetadata and ParselyVideoMetadata to Kotlin. Improve API.#96
Migrate ParselyMetadata and ParselyVideoMetadata to Kotlin. Improve API.#96
ParselyMetadata and ParselyVideoMetadata to Kotlin. Improve API.#96Conversation
BREAKING CHANGE: `toMap` were previously exposed to SDK consumers, but from SDK point of view, this was not necessary. To simplify the contract, those method are moved to be internal only. Also, the properties don't have to be `public` for the same reason.
`int` cannot be `null`, and `videoId` is annotated as `NonNull`
BREAKING CHANGE: instead of `java.utils.Calendar`, consumers now have to provide timestamp (in milliseconds) of publication date.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #96 +/- ##
==========================================
+ Coverage 68.24% 70.52% +2.27%
==========================================
Files 16 16
Lines 359 363 +4
Branches 52 53 +1
==========================================
+ Hits 245 256 +11
+ Misses 97 92 -5
+ Partials 17 15 -2 ☔ View full report in Codecov by Sentry. |
There's no need to force API consumers to use `ArrayList` - any `List` will be just fine.
| Calendar pubDate; | ||
| long publicationDateMilliseconds; |
There was a problem hiding this comment.
I'd appreciate feedback on this change. I don't like introducing a breaking change to the library, but at the same time, I would rather not force users to use java.util.Calendar API - there's no need to attach to this specific class to share a moment in time.
I've decided to propose a simple, API-agnostic, straightforward and universal long property. The consequence of this is that user can now easily provide an incorrect value (e.g. timestamp in seconds), but I sense that it's not that likely (name of the property) and simpler API has more value. WDYT?
There was a problem hiding this comment.
Could you elaborate on why using Calendar would be undesirable - besides the fact that it may not be the particular API the clients are using?
There was a problem hiding this comment.
The concern about different APIs used by the clients is the main one, but Calendar also feels heavier than what we need from this field, which is passing a timestamp.
But, as the majority of the usage is probably passing Calendar.getInstance() here, maybe it's not a big deal and backward compatibility and safer API design is better? I'm not sure now.
There was a problem hiding this comment.
In my opinion, in most cases using a type is better than not having a type at all. One obvious exception to that is when using a specific type requires the clients to add a new dependency. However, I believe java.util.Calendar is available without any external dependencies, so I don't see any issues about that.
Even if the type we choose is not exactly the type the clients would like to use, they can always convert to it on a need to basis as long as it is properly documented. In this case, Calendar is a well known and properly documented class, so I think it's a very good candidate to be used in external facing APIs.
There was a problem hiding this comment.
You're right - Calendar is a well-known and available dependency for every client. I agree it's easy to convert from any other API. Thank you for sharing your thoughts! - I brought back Calendar in 67a755b
In Java implementation, `pubDate` was `@Nullable` so there's no reason to change it in Kotlin implementation.
| private val authors: List<String>? = null, | ||
| @JvmField internal val link: String? = null, | ||
| private val section: String? = null, | ||
| private val tags: List<String>? = null, | ||
| private val thumbUrl: String? = null, | ||
| private val title: String? = null, | ||
| private val publicationDateMilliseconds: Long? = null |
There was a problem hiding this comment.
Instead of this, I was exploring introducing a builder pattern, so Java clients won't have to provide null in place of nullable fields.
I decided not doing it, as it'd deviate more from the original implementation, making addressing the breaking change more difficult. Furthermore, it'd be much different than iOS API. So I think, the default values for arguments will be just fine - making the API nicer for Kotlin users and not changing it for Java users.
There was a problem hiding this comment.
As far as I know, using Builder pattern would mean that we're adding to the API not changing it. We'd still have the ParselyMetadata without any breaking changes - but there would also be a ParselyMetadataBuilder which returns ParselyMetadata when we call .build() on it.
That's all to say that, if Builder pattern is something you'd like to utilize, I don't think it conflicts with the current implementation.
There was a problem hiding this comment.
That's true - thanks. I've tunneled vision on constructor OR builder, but there's no reason to not introduce builder separately from the constructor. Still, for now, I'd leave this as it is and keep the builder for possible enhancement in the future.
| * internet (i.e. app-only content) or if the customer is using an "in-pixel" integration. | ||
| * Otherwise, metadata will be gathered by Parse.ly's crawling infrastructure. | ||
| */ | ||
| open class ParselyMetadata |
There was a problem hiding this comment.
I always get a little worried when I see open keyword, but I guess this is keeping the current design - so it's all good 😅
There was a problem hiding this comment.
True 😃 but yes, that's intentional.
2 participants
Description
This PR migrates
ParselyMetadataandParselyVideoMetadatato Kotlin and improves its API so it's easier to use - especially for clients, which use Kotlin.Tests
There's no need to manually test this change. Unit tests should be fine.