-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
In-App Updates: Add Version model #23224
Conversation
📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
|
📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
|
07c8059
to
9cf33e8
Compare
func isLowerThanOrEqual(to anotherVersionString: String) -> Bool { | ||
[ComparisonResult.orderedSame, .orderedAscending].contains(self.compare(anotherVersionString, options: .numeric)) | ||
extension Version { | ||
init?(from versionString: String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nit) I would put this directly in the Version
file`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! d97c8d5
guard let version = Bundle.main.infoDictionary?["CFBundleShortVersionString"] as? String else { | ||
DDLogError("No CFBundleShortVersionString found in Info.plist") | ||
guard let version = Bundle.main.infoDictionary?["CFBundleVersion"] as? String else { | ||
DDLogError("No CFBundleVersion found in Info.plist") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nit) A good place for wpAssertionFailure
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! 6b62951
prereleaseIdentifiers: [String] = [], | ||
buildMetadataIdentifiers: [String] = [] | ||
) { | ||
precondition(major >= 0 && minor >= 0 && patch >= 0, "Negative versioning is invalid.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest replacing precondition
(crashes in prod) with safe failures and wpAssertionFailure
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! abb2724
9cf33e8
to
a45f6ae
Compare
@kean Thanks for reviewing - I've addressed all the comments. |
Fixes #23195 (review)
Description
How to test
AppUpdateCoordinatorTests
is all greenRegression Notes
Potential unintended areas of impact
n/a
What I did to test those areas of impact (or what existing automated tests I relied on)
n/a
What automated tests I added (or what prevented me from doing so)
n/a
PR submission checklist:
RELEASE-NOTES.txt
if necessary.Testing checklist: