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

Add MerchantRepository for holding BraintreeClient properties #1202

Merged
merged 3 commits into from
Nov 5, 2024

Conversation

tdchow
Copy link
Collaborator

@tdchow tdchow commented Nov 5, 2024

Summary of changes

  • Create MerchantRepository which holds properties that were previously in BraintreeClient
  • MerchantRepository singleton that keeps fields in memory, allowing any class to access its values via dependency injection

Checklist

  • Added a changelog entry
  • Relevant test coverage

Authors

List GitHub usernames for everyone who contributed to this pull request.

@tdchow tdchow requested a review from a team as a code owner November 5, 2024 16:17
import androidx.annotation.RestrictTo

@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
class MerchantRepository {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure if there's a clearer name for this class.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a doc string for our internal understanding of what this class is for?

Copy link
Contributor

Choose a reason for hiding this comment

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

➕ 1 I think some docstrings would help, I don't have any issues with the name since this naming seems like a common pattern on Android.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

does the @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) prevent dokka from publishing to the generated docs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like those internal classes do get published :(

https://braintree.github.io/braintree_android/BraintreeCore/com.braintreepayments.api.core/index.html

I'll look to see if there's a way to prevent this.

Copy link
Contributor

Choose a reason for hiding this comment

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

does the @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP) prevent dokka from publishing to the generated docs?

Not sure if it does that, but you could add @suppress in the docs to suppress it.
Also see: Kotlin/dokka#2404 for a custom implementation we can do to suppress all @RestrictTo annotated methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dang, I thought there'd be support directly in Dokka for that. I've added some docs for this class.

I'll create a tech debt ticket for possibly adding a custom implementation to suppress based on the annotation.

@tdchow tdchow merged commit ed09136 into main Nov 5, 2024
3 checks passed
@tdchow tdchow deleted the merchant-repository branch November 5, 2024 18:49
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.

5 participants