-
Notifications
You must be signed in to change notification settings - Fork 378
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
Xcode 7 fixes & modernization #76
Conversation
@@ -20,21 +20,21 @@ class NYTPhotosOverlayViewTests: XCTestCase { | |||
let overlayView = NYTPhotosOverlayView() | |||
let leftBarButtonItem = UIBarButtonItem(barButtonSystemItem: UIBarButtonSystemItem.Done, target: nil, action: nil) | |||
overlayView.leftBarButtonItem = leftBarButtonItem | |||
XCTAssert(leftBarButtonItem == overlayView.navigationBar.items.first?.leftBarButtonItem) | |||
XCTAssert(leftBarButtonItem == overlayView.navigationBar.items!.first?.leftBarButtonItem) |
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.
should we force unwrap here? Will this crash the tests if items
is nil? I am thinking maybe we can change the !
to a ?
so the test would just fail instead of crash if items
is nil.
FYI this swift business is new to me so let me know if I am reading this wrong or making incorrect assumptions.
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.
My thinking here was, it's a test, if it crashes so be it, that's still a failure. But maybe we ought to just let nil
come through and have the assertion fail. Will send an updated PR.
@cdzombak looks good: merging. In your next PR if you think this makes sense would u mind addressing? #76 (comment) |
Xcode 7 fixes & modernization
This PR accepts Xcode 7-recommend build settings changes and fixes problems revealed by some new warnings that are enabled by default in Xcode 7.
Other changes include:
Coming in a later PR: I need this PR as a foundation to keep working, but in my next PR I will address test failures introduced by dc75d98 .