Skip to content
This repository has been archived by the owner on Feb 17, 2021. It is now read-only.

Swift 3 branch without any API changes #3

Closed
wants to merge 10 commits into from
Closed

Swift 3 branch without any API changes #3

wants to merge 10 commits into from

Conversation

cxa
Copy link

@cxa cxa commented Jun 25, 2016

Hello, LayoutKit is awesome, but I would like to develop apps with Swift 3, unfortunately LayoutKit has no Swift 3 support yet, so I create a new branch for that reason. It's migrated by Xcode 8 beta, I don't touch any API names but I'm not sure if the migrations affect the API names, and fix some un-migratable errors manualy.

@@ -56,7 +57,7 @@ extension ReloadableViewLayoutAdapter: UICollectionViewDataSource {
arrangement = nil
assertionFailure("unknown supplementary view kind \(kind)")
}
arrangement?.makeViews(inView: view)
_ = arrangement?.makeViews(inView: view)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of this, can you mark makeViews as @discardableResult.

@nicksnyder
Copy link
Collaborator

Cool, thanks for doing this work! I added some comments to the code.

Can you also update .travis.yml to run Xcode8 so we can verify that tests pass?

osx_image: xcode8

Will also need to add/update the destinations appropriately in .travis.yml

};
name = Release;
};
0BE47F3C1CC57492003820E8 /* Debug */ = {
isa = XCBuildConfiguration;
buildSettings = {
ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES = YES;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain what this setting does? Does it replace EMBEDDED_CONTENT_CONTAINS_SWIFT?

Copy link
Author

Choose a reason for hiding this comment

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

You're right.
img

@nicksnyder
Copy link
Collaborator

Looking good, can you rebase on master so tests get run? I think there is a merge conflict because I modified .travis.yml in master due to some test flakiness.

@nicksnyder
Copy link
Collaborator

Looks like we need to remove iOS 8 & 9 simulator destinations from .travis.yml. Xcode 8 currently only includes iOS 10.

@nicksnyder
Copy link
Collaborator

Can you remove | xcpretty for the test command. It looks like something is failing during tests and we don't get a useful error message. Could be a bug in Xcode 8.

@cxa
Copy link
Author

cxa commented Jun 27, 2016

Still fails. Not sure how to deal this issue.

@nicksnyder
Copy link
Collaborator

Can you please turn off whole module optimization and also add --destination-timeout 300 to the xcodebuild commands in .travis.yml

@cxa
Copy link
Author

cxa commented Jun 27, 2016

None or Fast, Single-File Optimization, which option do you prefer?

@cxa
Copy link
Author

cxa commented Jun 27, 2016

And .travis.yml will be changed like this, is it OK?

language: objective-c
osx_image: xcode8
script:
  set -o pipefail &&
  xcodebuild build -project LayoutKit.xcodeproj -scheme LayoutKitSampleApp -sdk iphonesimulator10.0 --destination-timeout 300
    -destination 'platform=iOS Simulator,name=iPhone 6,OS=10.0'
    -destination 'platform=iOS Simulator,name=iPhone 6 Plus,OS=10.0' &&
  xcodebuild test -project LayoutKit.xcodeproj -scheme LayoutKit -sdk iphonesimulator10.0 --destination-timeout 300
    -destination 'platform=iOS Simulator,name=iPhone 6,OS=10.0'
    -destination 'platform=iOS Simulator,name=iPhone 6 Plus,OS=10.0'

after_success:
  - bash <(curl -s https://codecov.io/bash)

@nicksnyder
Copy link
Collaborator

.travis.yml looks good.

For optimization, I just want this line to disappear from the diff: SWIFT_OPTIMIZATION_LEVEL = "-Owholemodule". Default should be fast I think. It probably doesn't matter but I would like to minimize the diff when trying to debug issues like this.

@nicksnyder
Copy link
Collaborator

fyi, I pushed this as swift3 branch. Will continue to try to get test to pass on travis.

@cxa cxa closed this Aug 30, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants