Migrate `ParselyMetadata` and `ParselyVideoMetadata` to Kotlin. Improve API. #96

wzieba · 2023-11-29T18:11:56Z

Description

This PR migrates ParselyMetadata and ParselyVideoMetadata to 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.

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 · 2023-11-29T18:14:52Z

Codecov Report

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

Comparison is base (ee8614e) 68.24% compared to head (67a755b) 70.52%.

Files	Patch %	Lines
...com/parsely/parselyandroid/ParselyVideoMetadata.kt	27.27%	6 Missing and 2 partials ⚠️
...java/com/parsely/parselyandroid/ParselyMetadata.kt	87.50%	0 Missing and 3 partials ⚠️

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.
📢 Have feedback on the report? Share it here.

There's no need to force API consumers to use `ArrayList` - any `List` will be just fine.

wzieba · 2023-12-12T15:29:32Z

parsely/src/main/java/com/parsely/parselyandroid/ParselyMetadata.java

-    Calendar pubDate;
+    long publicationDateMilliseconds;


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?

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?

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.

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.

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

Thank you! 🙇‍♂️

In Java implementation, `pubDate` was `@Nullable` so there's no reason to change it in Kotlin implementation.

wzieba · 2023-12-12T15:41:26Z

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

+    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


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.

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.

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.

oguzkocer · 2023-12-12T18:42:44Z

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

+ * 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


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 😅

True 😃 but yes, that's intentional.

oguzkocer

@wzieba I left a couple replies to your line comments - but I don't have much else to add as this is a very straightforward PR. I'll mark this as approved once the conversations are resolved.

wzieba added 7 commits November 29, 2023 18:40

tests: add unit test for ParselyMetaData#toMap method

5563177

tests: add unit test for ParselyVideoMetaData#toMap method

597ff4c

style: address lint warnings

b7a9ba0

`int` cannot be `null`, and `videoId` is annotated as `NonNull`

refactor: change type of publication date from Calendar to long

341e73b

BREAKING CHANGE: instead of `java.utils.Calendar`, consumers now have to provide timestamp (in milliseconds) of publication date.

Rename .java to .kt

e1dbd50

refactor: migrate ParselyMetadata and ParselyVideoMetadata to kotlin

8119a56

wzieba added 2 commits December 12, 2023 15:27

Merge branch 'main' into issue/migrate_metadata_to_kotlin

baa1400

refactor: accept List in metadata's constructors

fa379a9

There's no need to force API consumers to use `ArrayList` - any `List` will be just fine.

wzieba commented Dec 12, 2023

View reviewed changes

wzieba added 2 commits December 12, 2023 16:35

fix: make publicationDateMilliseconds nullable

93c79ef

In Java implementation, `pubDate` was `@Nullable` so there's no reason to change it in Kotlin implementation.

feat: add null default arguments for nullable fields

810dec9

wzieba commented Dec 12, 2023

View reviewed changes

wzieba marked this pull request as ready for review December 12, 2023 16:20

wzieba added the enhancement label Dec 12, 2023

wzieba requested a review from oguzkocer December 12, 2023 16:21

oguzkocer reviewed Dec 12, 2023

View reviewed changes

wzieba requested a review from oguzkocer December 13, 2023 11:47

fix: bring back java.util.Calendar to metadata constructors

67a755b

oguzkocer approved these changes Dec 13, 2023

View reviewed changes

wzieba merged commit 7504512 into main Dec 13, 2023
2 checks passed

wzieba deleted the issue/migrate_metadata_to_kotlin branch December 13, 2023 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `ParselyMetadata` and `ParselyVideoMetadata` to Kotlin. Improve API. #96

Migrate `ParselyMetadata` and `ParselyVideoMetadata` to Kotlin. Improve API. #96

wzieba commented Nov 29, 2023 •

edited

codecov bot commented Nov 29, 2023 •

edited

wzieba Dec 12, 2023

oguzkocer Dec 12, 2023

wzieba Dec 13, 2023

oguzkocer Dec 13, 2023

wzieba Dec 13, 2023

oguzkocer Dec 13, 2023

wzieba Dec 12, 2023

oguzkocer Dec 12, 2023

wzieba Dec 13, 2023

oguzkocer Dec 12, 2023

wzieba Dec 13, 2023

oguzkocer left a comment

Migrate ParselyMetadata and ParselyVideoMetadata to Kotlin. Improve API. #96

Migrate ParselyMetadata and ParselyVideoMetadata to Kotlin. Improve API. #96

Conversation

wzieba commented Nov 29, 2023 • edited

Description

Tests

codecov bot commented Nov 29, 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

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

Migrate `ParselyMetadata` and `ParselyVideoMetadata` to Kotlin. Improve API. #96

Migrate `ParselyMetadata` and `ParselyVideoMetadata` to Kotlin. Improve API. #96

wzieba commented Nov 29, 2023 •

edited

codecov bot commented Nov 29, 2023 •

edited