Skip to content

Made some classes and functions internal#1289

Closed
vegaro wants to merge 0 commit into
bc6-support-newfrom
visibility
Closed

Made some classes and functions internal#1289
vegaro wants to merge 0 commit into
bc6-support-newfrom
visibility

Conversation

@vegaro

@vegaro vegaro commented Sep 25, 2023

Copy link
Copy Markdown
Member

In preparation for BC6 support, I've modified the visibility of some classes, constants and functions to make them internal. Most of them are in the amazon package (they used to be public to be able to call them from other modules)

Note this is based off #1209

@vegaro vegaro added the pr:breaking Changes that are breaking label Sep 25, 2023
@vegaro vegaro requested review from a team and MarkVillacampa September 25, 2023 18:00

// Public because it's been used in the hybrids
fun warnLog(message: String) {
internal fun warnLog(message: String) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we can make this one internal even if it's been used in the hybrids

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No... it's only two logs though... maybe we can do something so we can hide this in this SDK 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Well, if we make it internal, we can still log things in PHC/Hybrids... The consequence of that however is that those logs won't be redirected through the log handler in case the developer has set any. If we want to bypass that, we should wrap the log handler in PHC to set it in Android but also to redirect the messages in PHC. But honestly, if those logs are not very important, I would be ok if they are not redirected for now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, I agree, I'd rather make this internal so we don't expose this to devs for no reason

@codecov

codecov Bot commented Sep 25, 2023

Copy link
Copy Markdown

Codecov Report

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

Comparison is base (46bd9b2) 85.61% compared to head (aaecfd1) 85.61%.
Report is 1 commits behind head on bc6-support-new.

Additional details and impacted files
@@               Coverage Diff                @@
##           bc6-support-new    #1289   +/-   ##
================================================
  Coverage            85.61%   85.61%           
================================================
  Files                  185      185           
  Lines                 6313     6313           
  Branches               916      916           
================================================
  Hits                  5405     5405           
  Misses                 557      557           
  Partials               351      351           
Files Coverage Δ
...n/com/revenuecat/purchases/amazon/AmazonBackend.kt 86.66% <ø> (ø)
...lin/com/revenuecat/purchases/amazon/AmazonCache.kt 85.71% <ø> (ø)
...rchases/amazon/DefaultPurchasingServiceProvider.kt 14.28% <ø> (ø)
...azon/attribution/AmazonDeviceIdentifiersFetcher.kt 100.00% <100.00%> (ø)
...cat/purchases/amazon/handler/ProductDataHandler.kt 89.65% <100.00%> (ø)
...nuecat/purchases/amazon/handler/PurchaseHandler.kt 88.88% <100.00%> (ø)
...purchases/amazon/handler/PurchaseUpdatesHandler.kt 91.66% <100.00%> (ø)
...ses/amazon/listener/ProductDataResponseListener.kt 0.00% <ø> (ø)
...chases/amazon/listener/PurchaseResponseListener.kt 0.00% <ø> (ø)
...amazon/listener/PurchaseUpdatesResponseListener.kt 0.00% <ø> (ø)
... and 9 more

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

@tonidero tonidero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❤️

@vegaro

vegaro commented Sep 26, 2023

Copy link
Copy Markdown
Member Author

Let's keep this open until the base branch is ready to go

@MarkVillacampa

Copy link
Copy Markdown
Member

I've cherry-picked the commit in this PR into the #bc6-support-new branch because, after force-pushing that branch, there were a bunch of conflicts here.

@vegaro vegaro deleted the visibility branch February 15, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:breaking Changes that are breaking

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants