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

Swift 3 #30

Closed
wants to merge 78 commits into from
Closed

Swift 3 #30

wants to merge 78 commits into from

Conversation

phatblat
Copy link
Collaborator

@phatblat phatblat commented Oct 21, 2016

Continuing the work started in #20, but with support for only Swift 3.0.

TODO

  • Remove Swift 2 support
  • Update generic method where clause to non-deprecated syntax
  • Test with CocoaPods 1.1.1
  • Update README.md
  • Update CHANGELOG.md

Releases

The current plan for releasing Swift 3 support:

Fixes the following error in "Copy Swift standard libraries into Pods_ReusableDemo.framework":
Found an unexpected Mach-O header code: 0x72613c21
Swift 2 warnings introduced with Swift 3 support
CocoaPods fails with the following message unless the iOS 9.3 Simulator is installed (1.5GB):
- ERROR | [iOS] unknown: Encountered an unknown error (Simulator iPhone 4s is not available.
syntax differs between swift versions
This reverts commit 5fb0ab2.
# Conflicts:
#	.travis.yml
#	CHANGELOG.md
#	Example/ReusableDemo/TableViewCells/MyXIBSwitchCell.swift
#	Example/ReusableDemo/TableViewController.swift
#	Rakefile
@@ -4,3 +4,9 @@ use_frameworks!
target 'ReusableDemo' do
pod 'Reusable', :path => '../'
end

post_install do |installer|
installer.pods_project.build_configurations.each do |config|
Copy link
Owner

Choose a reason for hiding this comment

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

Reminder: you'll probably be able to get rid of that when you'll migrate to CP 1.1.1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@phatblat
Copy link
Collaborator Author

This is ready for review.

Copy link
Collaborator

@Grubas7 Grubas7 left a comment

Choose a reason for hiding this comment

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

Just minor remarks :)

@IBAction func closeAction(sender: UIButton) {
self.presentingViewController?.dismissViewControllerAnimated(true, completion: nil)
@IBAction func closeAction(_ sender: UIButton) {
self.presentingViewController?.dismiss(animated: true, completion: nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can omit , completion: nil. It's optional.

// This is an example of a functional test case.
// Use XCTAssert and related functions to verify your tests produce the correct results.
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Useless file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it's just the auto-generated example tests file. I started on #7 in this effort, but then decided to wait until the API settled before actually writing tests.

@@ -28,7 +28,7 @@ public protocol StoryboardBased: class {
public extension StoryboardBased {
/// By default, use the storybaord with the same name as the class
static var storyboard: UIStoryboard {
return UIStoryboard(name: String(self), bundle: NSBundle(forClass: self))
return UIStoryboard(name: String(describing: self), bundle: Bundle(for: self))
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use Self().sceneIdentifier and replace String(describing: self).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like StoryboardBased doesn't have a sceneIdentifier class property like StoryboardSceneBased. Seems like an oversight since any view controller can have a storyboard ID.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏻

Copy link
Owner

Choose a reason for hiding this comment

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

StoryboardBased is designed for the case where you have an initialViewController with the same name as the Storyboard file. As it instantiate the VC using initialViewController (and not using a sceneIdentifier), having a sceneIdentifier for the StoryboardBased shouldn't be a requirement (as it's optional, and not required for the StoryboardBased mixin to work)

fatalError("The viewController '\(self.sceneIdentifier)' of '\(storyboard)' is not of class '\(self)'")
let sb = Self().storyboard

let vc = sb?.instantiateViewController(withIdentifier: self.sceneIdentifier)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most of let in other methods have well descriptive names. Here are sb, vc, typedVC. IMO you could use full names ;)

let detailsVC = InfoDetailViewController.instantiate()
detailsVC.setDetails(self.details)
self.window?.rootViewController?.presentViewController(detailsVC, animated: true, completion: nil)
self.window?.rootViewController?.present(detailsVC, animated: true, completion: nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

, completion: nil is optional.

@phatblat
Copy link
Collaborator Author

Thanks for the feedback, @Grubas7!

@AF-cgi
Copy link

AF-cgi commented Nov 6, 2016

When do you merge this?

@AliSoftware
Copy link
Owner

AliSoftware commented Nov 6, 2016

@phatblat If this is ready, please:

  • rebase on top of master (which now contains the 2.3 conversion)
  • and maybe cleanup the git history as it seems to have picked up a lot of history? idk, 78 commits seems like a lot probably caused by the multiple rebasing given the history of this PR, so some cleaning could make it easier to read the git history?
  • Add a Swift 3.0 badge Swift 3.0 at the top of the README instead of the 2.2 & 2.3 ones I added in master

So we could merge soon 👍

ceyhun pushed a commit to ceyhun/Reusable that referenced this pull request Nov 9, 2016
@AliSoftware
Copy link
Owner

I'm closing this one in favour of #31 .

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.

4 participants