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

Migrated to Swift 4 #199

Merged
merged 6 commits into from
Nov 22, 2017
Merged

Migrated to Swift 4 #199

merged 6 commits into from
Nov 22, 2017

Conversation

jay18001
Copy link
Contributor

@jay18001 jay18001 commented Nov 2, 2017

Migrated to swift 4.0

  • Update the Framework
  • Update the Sample App
  • Update the Playgrounds
  • Update samples in Readme

closes #198

NSForegroundColorAttributeName: Theme.textColorForNavigationTitle,
NSFontAttributeName: Theme.fontForNavigationTitle
NSAttributedStringKey.foregroundColor: Theme.textColorForNavigationTitle,
NSAttributedStringKey.font: Theme.fontForNavigationTitle
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to remove the NSAttributedStringKey and just use .foregroundColor and .font

NSForegroundColorAttributeName: Theme.textColorForNavigationTitle,
NSFontAttributeName: Theme.fontForNavigationTitle
NSAttributedStringKey.foregroundColor: Theme.textColorForNavigationTitle,
NSAttributedStringKey.font: Theme.fontForNavigationTitle
Copy link
Contributor

Choose a reason for hiding this comment

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

same here, remove the NSAttributedStringKey type whenever possible

@@ -257,7 +257,7 @@ open class BrickCell: BaseBrickCell {
open func heightForBrickView(withWidth width: CGFloat) -> CGFloat {
self.layoutIfNeeded()

let size = self.systemLayoutSizeFitting(CGSize(width: width, height: 0), withHorizontalFittingPriority: 1000, verticalFittingPriority: 10)
let size = self.systemLayoutSizeFitting(CGSize(width: width, height: 0), withHorizontalFittingPriority: UILayoutPriority.required, verticalFittingPriority: UILayoutPriority(rawValue: 10))
Copy link
Contributor

Choose a reason for hiding this comment

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

just use .required

XCTAssertEqual(cell2!.transform.scaleX, 5/6, accuracy: 0.01)
XCTAssertEqual(cell2!.transform.scaleY, 5/6, accuracy: 0.01)
XCTAssertEqual(cell3!.transform.scaleX, 5/6, accuracy: 0.01)
XCTAssertEqual(cell3!.transform.scaleY, 5/6, accuracy: 0.01)
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 you might be able to use ? instead of ! for these, so that if the cell is nil it wont crash the test runner. can you try changing that and seeing if it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, that gives a error, All the prams have to be not optional on XCTAssertEqual when using accuracy

Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be to do let cell1 = brickView.cellForItem(at: IndexPath(item: 0, section: 1))! instead of adding cell! over and over. The problem, however is that it would crash if there's no cell instead of triggering a test failure (which is arguably a bit more graceful). So even better would be to guard let cell1 = .. else { XCTFail() } because you can provide a message.

XCTAssertEqual(cell2!.transform.scaleX, 5/6, accuracy: 0.01)
XCTAssertEqual(cell2!.transform.scaleY, 5/6, accuracy: 0.01)
XCTAssertEqual(cell3!.transform.scaleX, 0.5, accuracy: 0.01)
XCTAssertEqual(cell3!.transform.scaleY, 0.5, accuracy: 0.01)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

thirdAttributes = layout.layoutAttributesForItem(at: IndexPath(item: 2, section: 0))
XCTAssertEqualWithAccuracy(thirdAttributes!.frame, CGRect(x: 0, y: 400, width: 320, height: 100), accuracy: CGRect(x: 1, y: 1, width: 1, height: 1))
XCTAssertEqual(thirdAttributes!.frame, CGRect(x: 0, y: 400, width: 320, height: 100), accuracy: CGRect(x: 1, y: 1, width: 1, height: 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

same in this file if you can use ?

@codecov-io
Copy link

codecov-io commented Nov 17, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@daf9dd4). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master    #199   +/-   ##
========================================
  Coverage          ?   92.7%           
========================================
  Files             ?      40           
  Lines             ?    4513           
  Branches          ?     375           
========================================
  Hits              ?    4184           
  Misses            ?     328           
  Partials          ?       1
Impacted Files Coverage Δ
Source/Bricks/Generic/GenericBrick.swift 99.31% <100%> (ø)
Source/Cells/BrickCell.swift 92.4% <100%> (ø)
Source/Bricks/Collection/CollectionBrick.swift 86.04% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update daf9dd4...b86d02b. Read the comment docs.

@ablokker
Copy link
Contributor

Just make sure to squash this into a single "Migrated to Swift 4" commit before merging to make it cleaner.

@jay18001 jay18001 merged commit 74a320c into master Nov 22, 2017
@jay18001 jay18001 deleted the swift-4 branch November 22, 2017 16:49
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.

Upgrade to swift 4
4 participants