-
Notifications
You must be signed in to change notification settings - Fork 71
Feat: Update Minimum Release Version and Remove Deprecated Code #460
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
base: blackout/Phase-1
Are you sure you want to change the base?
Feat: Update Minimum Release Version and Remove Deprecated Code #460
Conversation
9b7d56d to
5878366
Compare
nickolas-dimitrakas
left a comment
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.
LGTM
e756cb4 to
c607910
Compare
c607910 to
b4767f1
Compare
denischilik
left a comment
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.
A lot of great cleanup and modernization work is done here, but the PR is quite large and very difficult to review safely. Some parts also appear to modify behavior, not just update deployment targets or remove deprecated APIs.
To make the review clearer and safer, could we split this into three focused PRs:
- deployment target and podspec updates,
- removal of deprecated public APIs,
- internal logic updates (persistence, networking, utilities)?
5878366 to
8945413
Compare
| do { | ||
| return try NSKeyedUnarchiver.unarchivedObject(ofClass: MPUploadSettings.self, from: data) | ||
| } catch { | ||
| print(error) |
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: Should these prints be going to logger instead?
|
I broke this into three commits and corresponding PRs based off Denis' request. Thanks for looking @denischilik @nickolas-dimitrakas @jamesnrokt @rmi22186 I'm working on James' nit now but I've gotta inject the logger dependency into MPUserDefaults.swift and that gets initialized in a bunch of odd places so it may take a little time (thankfully its in the 3rd of the 3 prs) feat: Update Minimum Deployment Targets |
The merge-base changed after approval.
Background
What Has Changed
Checklist
Additional Notes
Reference Issue (For employees only. Ignore if you are an outside contributor)