-
Notifications
You must be signed in to change notification settings - Fork 997
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 minimum requirements to Xcode9 / iOS 9 #851
Conversation
The faux pas 1.7.2 problem seems to only be with the dynamic framework target, so we can continue to lint the static lib target to get some checking done. Also disable the APIAvailability rule since with xcode 9 this is built in with `@available` (which FauxPas doesn't recognize)
Audited all our `respondsToSelector` and FauxPas `APIAvailability` checks. Removed any unnecessary ones (now that we are on 9.0) and replaced all the relevant ones with `@available` checks instead.
Our custom version doesn't work correctly on iOS11 and I don't see a reason to not just use the default system behavior for this.
Make it public correctly. Replace all existing manual fauxpas defines with header import. Remove unnecssary defines/imports.
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.
just one question about the STPCoreScrollViewController
change, otherwise looks good!
--target "StripeiOS" \ | ||
--configFile "${config_path}" \ | ||
--minErrorStatusSeverity "${min_severity}" | ||
# Fauxpas 1.7.2 does not build our framework target correctly https://bitbucket.org/ali_rantakari/faux-pas/issues/117/unrecoverable-compiler-error-translation |
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.
ah, interesting that it's only the dynamic framework. guess we'll probably get most of the same warnings from linting just the static framework so that's good.
@@ -0,0 +1,40 @@ | |||
// | |||
// FauxPasAnnotations.h |
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.
👍
@@ -46,21 +54,4 @@ - (void)updateAppearance { | |||
} | |||
} | |||
|
|||
- (void)viewWillAppear:(BOOL)animated { |
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.
does removing this break things on iOS < 11?
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.
Not that I can tell. iOS 9&10 still have automatic content insetting (automaticallyAdjustsScrollViewInsets
). I changed this setting from off to on in STPCoreViewController. I'm not clear on why we had custom code to do this. Possibly the auto insetting was not present or buggy at some point? iOS 7 or 8? It seems to work correctly now, I played around in the 10.3.1 simulator. I can test in a 9.x simulator as well.
The new iOS11 replacement for automaticallyAdjustsScrollViewInsets
is contentInsetAdjustmentBehavior
and it is on the UIScrollView itself, which is why the check is in STPCoreScrollViewController
's viewDidLoad
. Whereas the call to automaticallyAdjustsScrollViewInsets
is on UIViewController
so it is in STPCoreViewController
.
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 just ran it in the 9.3 simulator and it seems to work fine there also. I thought perhaps the reason we had custom code was because of our custom insetting logic around keyboard insetting, but that all seems to work fine too with using the built in Apple insetting behavior, so I have no idea.
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.
cool, sgtm – thanks for the explanation!
#import "STPCoreScrollViewController+Private.h" | ||
|
||
@implementation FBSnapshotTestCase (STPViewControllerLoading) | ||
- (UIView *)stp_preparedAndSizedViewForSnapshotTestFromViewController:(UIViewController *)viewController { |
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.
nice 👍
MIGRATING.md
Outdated
@@ -1,5 +1,9 @@ | |||
## Migration Guides | |||
|
|||
### Migrating from versions < 12.0.0 | |||
* The SDK now requires iOS 9+ andXcode version is now 9+. If you need to support iOS 8 or Xcode 8, the last supported version is [11.5.0](https://github.com/stripe/stripe-ios/releases/tag/v11.5.0) |
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.
typo: andXcode
Despite Xcode recommending this change, it does not work correctly for some of our integration patterns. Thankfully our static framework validator CI script specifically checks for this case and yelled at me.
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!
This is a pretty monster change because so many things needed to change to get it working, but I have tried to split each commit up into a logical chunk so you may want to review them one at a time.
cc @danj-stripe (in case you also wanted to skim the changes).
Summary of changes:
respondsToSelector
checks and FauxPasAPIAvailability
rule with Xcode9's@available
function and warning.