Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prepare ParselyTracker for Kotlin migration #98

Merged
merged 9 commits into from
Dec 13, 2023

Conversation

wzieba
Copy link
Collaborator

@wzieba wzieba commented Dec 13, 2023

Description

This PR is a set of changes that aim to reduce the size of ParselyTracker to reduce the responsibilities of this class and make the transition from Java to Kotlin more readable and easier to review.

Testing

I see no need for manual tests of this PR. Unit and functional tests suite should be just fine.

Reasons: to reduce responsibilities of `ParselyTracker` and introduce unit tests coverage for "no internet connection" case
Instead of returning `-1` in case of missing interval
BREAKING CHANGE: this method was part of the public contract but I can't find a justified value of it
Copy link

codecov bot commented Dec 13, 2023

Codecov Report

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

Comparison is base (ee8614e) 68.24% compared to head (7fdcbbc) 69.25%.

Files Patch % Lines
...ava/com/parsely/parselyandroid/ParselyTracker.java 30.00% 7 Missing ⚠️
...m/parsely/parselyandroid/LocalStorageRepository.kt 0.00% 2 Missing ⚠️
...rsely/parselyandroid/ConnectivityStatusProvider.kt 80.00% 0 Missing and 1 partial ⚠️
...rc/main/java/com/parsely/parselyandroid/Logging.kt 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
+ Coverage   68.24%   69.25%   +1.00%     
==========================================
  Files          16       18       +2     
  Lines         359      361       +2     
  Branches       52       52              
==========================================
+ Hits          245      250       +5     
+ Misses         97       95       -2     
+ Partials       17       16       -1     

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


import java.util.Formatter

object Logging {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this moment I can't make this object internal because it won't be visible for Java classes. When the migration from Java to Kotlin is completed, I'll make sure to make it internal.

In case of an empty `url` parameter, the SDK should not crash the application. The logging SDK is likely not as critical to the consumer business flow to the degree, that providing an incorrect argument would kill the whole process.
@wzieba wzieba marked this pull request as ready for review December 13, 2023 16:59
@wzieba wzieba requested a review from oguzkocer December 13, 2023 16:59
Copy link
Collaborator

@oguzkocer oguzkocer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@wzieba wzieba merged commit fe141cf into main Dec 13, 2023
2 checks passed
@wzieba wzieba deleted the issue/prepare_for_parsely_tracker_kotlin_migration branch December 13, 2023 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants