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.

Sign up for GitHub

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

Jump to bottom

Migrates RCPurchases #758

Merged
merged 10 commits into from
Aug 23, 2021
Merged

Migrates RCPurchases #758

merged 10 commits into from
Aug 23, 2021

Conversation

taquitos
Copy link
Contributor

Resolves #546

@taquitos
Start of migration
2ccc575
@taquitos
Migrates RCPurchases
977c116 
Resolves #546 (last file to be migrated)
@taquitos taquitos requested review from a team and jeiting August 20, 2021 23:27
@taquitos
Copy link
Contributor Author

Adding @jeiting to review the ObjC updates.

@taquitos taquitos linked an issue Aug 20, 2021 that may be closed by this pull request
Migrate RCPurchases #546
Closed
aboedo
aboedo reviewed Aug 23, 2021
View reviewed changes
Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

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

great work so far! I'm still reviewing, left a few comments

PublicSDKAPITester/RCEntitlementInfoAPI.m Show resolved Hide resolved
PurchasesCoreSwift/Logging/Strings/Strings.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Misc/PurchasesTestStandIn.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Misc/PurchasesTestStandIn.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Public/Purchases.swift Show resolved Hide resolved
PurchasesCoreSwift/Public/Purchases.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Public/Purchases.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Public/Purchases.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Public/Purchases.swift Outdated Show resolved Hide resolved
PurchasesCoreSwift/Public/Purchases.swift Outdated Show resolved Hide resolved
@aboedo
Copy link
Member

aboedo commented Aug 23, 2021 •
edited
Loading

tagging @vegaro and @beylmk as well, since the more eyes the better for this one

@taquitos taquitos mentioned this pull request Aug 23, 2021
Closed
Juanpe
Juanpe reviewed Aug 23, 2021
View reviewed changes
PurchasesTests/Purchasing/PurchasesTests.swift Outdated
assertionHappened.fulfill()
}
let purchases = Purchases.shared
print(purchases)
Copy link
Contributor

@Juanpe Juanpe Aug 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Wow, I really like the approach 👌🏼 IMO, modifying the production code and adding testing staff is not good but I think in this case it's necessary. I was thinking of another approach, something like @_dynamicReplacement but I couldn't get anything better ¯_(ツ)_/¯

giphy

BTW, I think this print method could be removed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I totally agree, adding in code in prod to make testing better is almost always a sign we should refactor something
Also, that print method, whooooops.

@taquitos taquitos merged commit 444a979 into swift_migration Aug 23, 2021
@taquitos taquitos deleted the purchases branch August 23, 2021 21:00
@taquitos taquitos mentioned this pull request Aug 26, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Juanpe Juanpe Juanpe left review comments

@beylmk beylmk beylmk left review comments

@aboedo aboedo aboedo left review comments

@jeiting jeiting Awaiting requested review from jeiting

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Migrate RCPurchases
4 participants
@taquitos @aboedo @Juanpe @beylmk