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

Move ParselyAPIConnection to Kotlin #88

Merged
merged 6 commits into from
Nov 8, 2023
Merged

Conversation

wzieba
Copy link
Collaborator

@wzieba wzieba commented Nov 6, 2023

Description

This small PR moves ParselyAPIConnection to Kotlin. I need it to be in Kotlin to introduce migrate from AsyncTask used in this class to Kotlin Coroutines. So please note, that this class will be heavily rewritten in next PRs.

How to test

As this change changes code in HTTP communication, which is mocked in our tests, please:

  • Update MainActivity.java#31 and make the sample app not in the debug mode
  • Tap on "track url"
  • Move app to the background
  • See in logs that POST request was successful

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

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

Comparison is base (fd545a6) 51.63% compared to head (e2e6512) 51.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
- Coverage   51.63%   51.22%   -0.41%     
==========================================
  Files          10       10              
  Lines         368      367       -1     
  Branches       55       56       +1     
==========================================
- Hits          190      188       -2     
  Misses        165      165              
- Partials       13       14       +1     
Files Coverage Δ
...com/parsely/parselyandroid/ParselyAPIConnection.kt 91.66% <91.66%> (ø)

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

@wzieba wzieba force-pushed the parsely_api_connection_to_kotlin branch from df47843 to cc43b9f Compare November 6, 2023 16:25
We don't need such check in this SDK. Also, it can be false positive when moving from Java to Kotlin
@wzieba wzieba force-pushed the parsely_api_connection_to_kotlin branch from cc43b9f to 5769882 Compare November 6, 2023 16:26
@wzieba wzieba requested a review from ParaskP7 November 7, 2023 10:05
Copy link
Collaborator

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @wzieba !

I have reviewed and tested this PR as per the instructions, everything works as expected, good job! 🌟


I have left a questions (❓) and some minor (🔍) comments for you to consider. I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.

@wzieba wzieba merged commit 1e6e189 into main Nov 8, 2023
2 checks passed
@wzieba wzieba deleted the parsely_api_connection_to_kotlin branch November 8, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants