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

Change Strings to enums #769

Merged
merged 8 commits into from
Aug 25, 2021

Conversation

Juanpe
Copy link
Contributor

@Juanpe Juanpe commented Aug 24, 2021

Resolves #768

The goal of this PR is to convert the Strings classes to enums based on this discussion.

Moreover, I've updated some failed tests for devices with versions prior to 12.2. I'll give you more details throughout the code.

Thank you for contributing to Purchases. Before pressing the "Create Pull Request" button, please provide the following:

  • A description about what and why you are contributing, even if it's trivial.
  • The issue number(s) or PR number(s) in the description if you are contributing in response to those.
  • If applicable, unit tests.

@@ -212,7 +212,7 @@ class ProductInfoExtractorTests: XCTestCase {

let receivedProductInfo = productInfoExtractor.extractInfo(from: product)

expect(receivedProductInfo.discounts).to(beEmpty())
expect(receivedProductInfo.discounts).to(beNil())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extractor returns nil instead of an empty array

@@ -181,14 +181,14 @@ class ProductInfoExtractorTests: XCTestCase {

let receivedProductInfo = productInfoExtractor.extractInfo(from: product)

expect(receivedProductInfo.subscriptionGroup).to(beEmpty())
expect(receivedProductInfo.subscriptionGroup).to(beNil())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The extractor returns nil instead of an empty array

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -40,7 +40,7 @@ class MockSKProduct: SKProduct {
@available(iOS 11.2, tvOS 11.2, macOS 10.13.2, *)
lazy var mockDiscount: SKProductDiscount? = nil

@available(iOS 11.2, tvOS 11.2, macOS 10.13.2, *)
@available(iOS 12.2, tvOS 12.2, macOS 10.13.2, *)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the main change related to the failed tests. The problem is that although SKProductDiscount is available from iOS 11.2, the property discounts is available from iOS 12.2 😓 So, we have to update the availability of this property and the derived tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch!

static let purchaserInfo = PurchaserInfoStrings.self
static let receipt = ReceiptStrings.self
static let restore = RestoreStrings.self

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In order not to make a lot of changes in the codebase, I left these "aliases" there.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think leaving the aliases in this PR makes sense, and we can do another PR to remove them and change the code.

Conflicts:
	PurchasesCoreSwift/Logging/Strings/AttributionStrings.swift
	PurchasesCoreSwift/Logging/Strings/ConfigureStrings.swift
	PurchasesCoreSwift/Logging/Strings/NetworkStrings.swift
	PurchasesCoreSwift/Logging/Strings/OfferingStrings.swift
	PurchasesCoreSwift/Logging/Strings/PurchaserInfoStrings.swift
	PurchasesCoreSwift/Logging/Strings/ReceiptStrings.swift
	PurchasesCoreSwift/Logging/Strings/RestoreStrings.swift
	PurchasesCoreSwift/Logging/Strings/Strings.swift
@Juanpe Juanpe marked this pull request as ready for review August 24, 2021 21:37
@@ -22,6 +22,7 @@ extension String {

}

// swiftlint:disable identifier_name
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exist a var called i, and due to that, it's breaking this rule.

In fact, there are other parts of the code that break this rule as well. I found these:

  • Data+Extensions.swift:35
  • Data+Extensions.swift:40
  • SwiftStyleGuide.swift:140

These errors will be fixed when this rule will be enabled again, I guess

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm working on enabling swiftlint right now, so these will be addressed very soon!

@taquitos taquitos requested a review from a team August 25, 2021 16:39
Copy link
Contributor

@taquitos taquitos left a comment

Choose a reason for hiding this comment

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

🥅 ⚽ 🐐

@Juanpe
Copy link
Contributor Author

Juanpe commented Aug 25, 2021

Thanks for the review 🙂

@taquitos taquitos merged commit a2bec97 into RevenueCat:swift_migration Aug 25, 2021
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