Converted library and Demo project to Swift 3 #31
Conversation
- Updated podspec version to 3.0.0 - Updated README and Changelog - Removed `ALWAYS_EMBED_SWIFT_STANDARD_LIBRARIES` build setting - Updated `.travis.yml` and `.swift-version`
…ueueReusableCell(for:cellType:)` as it was done in AliSoftware#30
|
@Grubas7 I am aware of #30 but it has some conflicts with |
|
It seems that the CocoaPods version used by Travis isn't recent enough to catch the @ceyhuno Could you add |
| @@ -18,8 +18,8 @@ public extension UICollectionView { | |||
|
|
|||
| - seealso: `registerNib(_:,forCellWithReuseIdentifier:)` | |||
| */ | |||
| final func registerReusableCell<T: UICollectionViewCell where T: NibReusable>(cellType: T.Type) { | |||
| self.registerNib(cellType.nib, forCellWithReuseIdentifier: cellType.reuseIdentifier) | |||
| final func registerReusableCell<T: UICollectionViewCell>(_ cellType: T.Type) where T: NibReusable { | |||
There was a problem hiding this comment.
Wondering if, to comply to the Swift 3.0 API design guidelines, we shouldn't declare that as register<T: UICollectionViewCell>(cellType: T.Type) where T: NibReusable instead (so that call site looks like collectionView.register(cellType: MyXIBCell.self)?
Of course same comment and reflection applies to all the methods for UICollectionView and UITableView.
|
I'm not sur what's happening on Travis. Analysing again the logs of the previous build that failed (th one before your commit to add
|
…the Swift 3 API design guidelines
|
Previous build passed! The error was in fact due to It seems you already have fixed that (probably amended and force-pushed the fix since I don't s it anymore in the commit list of this PR 👍 ) and now use So the language: swift
before_install:
- which swiftlint || brew install swiftlint
- gem install cocoapods
osx_image: xcode8
matrix:
include:
- env:
- TASK=lintpod
- env:
- TASK=carthage
script:
- rake ci:$TASK |
|
Seems we're good now 🎉 Given that this Swift 3 migration also means having an API compliant with Swift 3 guidelines, and that we can't mess this up for 3.0.0 (We don't want to forget to Swiftify one API in 3.0.0 and have to bump to 4.0.0 just to fix that and make a breaking change), I think it's important to double-check them. At first glance they seem OK, but we don't want to miss one. So I think that would be cool if multiple persons could just take some time to double-check what the public APIs look like after that change and all approve if they're compliant with the expected Swift 3 guidelines (or suggest changes) before we really merge that PR — as I don't want to mess that up, and aim at a clean Swift 3 API 😉 Once 2 or 3 people give their 👍 about how the new API look like, we'll be ready to merge… and ship! |
| @@ -1,5 +1,16 @@ | |||
| # CHANGELOG | |||
|
|
|||
| ## UNRELEASED | |||
|
|
|||
| * Converted library and Demo project to Swift 3. | |||
There was a problem hiding this comment.
Hey @ceyhuno, you forgot to credit yourself! (I think the other people who worked on the Swift 3 migration like @phatblat on #30 deserve credits there too, btw)
Could you add there the links to your profiles and to the PRs related to that migration? (you can use other entries in the CHANGELOG as example)
| (elementKind: String, indexPath: NSIndexPath, viewType: T.Type = T.self) -> T { | ||
| let view = self.dequeueReusableSupplementaryViewOfKind(elementKind, withReuseIdentifier: viewType.reuseIdentifier, forIndexPath: indexPath) | ||
| final func dequeueReusableSupplementaryView<T: UICollectionReusableView> | ||
| (ofKind elementKind: String, indexPath: IndexPath, viewType: T.Type = T.self) -> T where T: Reusable { |
There was a problem hiding this comment.
I think the new API for this should be dequeueReusableSupplementaryView(ofKind:for:viewType:) instead?
| @@ -8,8 +8,7 @@ A Swift mixin to use `UITableViewCells`, `UICollectionViewCells` and `UIViewCont | |||
|
|
|||
| [](http://cocoadocs.org/docsets/Reusable) | |||
| [](http://cocoadocs.org/docsets/Reusable) | |||
|  | |||
|  | |||
| [](https://swift.org) | |||
There was a problem hiding this comment.
For accessibility reasons (= because people with visual deficiency use the alt description of an image to help them understand what the image is) I think it would be better for non-automatic badges like this one (unlike the one whose image is auto-generated) to have a descriptive text, like Language: Swift 3.0 instead of just "Language"?
…` to `dequeueReusableSupplementaryView(ofKind:for:viewType:)` Updated alt description of language badge in README
|
One thing that could be a good idea to add in the README is that "if you still need Swift 2.x support, you should use Reusable 2.5.1" somewhere |
|
@AliSoftware I see that most of my remarks from #30 (useless completion blocks, descriptive var names) are not applied to this PR. Looks like most of Please take a look at #30 remarks, and then I'll review this PR one more time :) |
There was a problem hiding this comment.
@Grubas7 's remarks on #30 to be applied to this #31 :
- Optional completions: Swift 3 #30 (comment) & Swift 3 #30 (comment)
- Descriptive variable names: Swift 3 #30 (comment)
- Revert the changes done by the migrator from
privatetofileprivatein the Example project, asprivateshould be enough in those cases.
(First two are not related to the Swift 3 migration per se but would be a good occasion to cleanup the core indeed. 3rd is more related to the migration even if it's only affecting the sample project.)
|
I think all of these changes can be made in future pull requests and/or commits without the need to change the MAJOR/MINOR version of the framework. We shouldn't do code cleanup or non-framework-related changes during a migration and make this pull request bigger than it's supposed to be. |
|
@ceyhuno I do agree with you… except maybe for the last point (fileprivate->private) which is a change made by this PR while it shouldn't have been necessary? |
|
👍 I'll revert |
| if let view = view as? UIView { | ||
| view.translatesAutoresizingMaskIntoConstraints = false | ||
| owner.addSubview(view) | ||
| layoutAttributes.forEach { attribute in | ||
| owner.addConstraint(NSLayoutConstraint(item: view, | ||
| attribute: attribute, | ||
| relatedBy: .Equal, | ||
| relatedBy: .equal, | ||
| toItem: owner, | ||
| attribute: attribute, | ||
| multiplier: 1, |
There was a problem hiding this comment.
multiplier is CGFloat so I would do 1.0 instead of 1, but it's not crucial ;)
There was a problem hiding this comment.
Good point, but not related to the migration, so probably should fix in a separate PR 😉
| @@ -18,8 +18,8 @@ public extension UICollectionView { | |||
|
|
|||
| - seealso: `registerNib(_:,forCellWithReuseIdentifier:)` | |||
There was a problem hiding this comment.
Docs for this and UITableView extension still refers to Swift 2.x
-> register(_:forCellReuseIdentifier:)
|
I checked |
|
Cool! I'd then say once the last doc typo is fixed it'll be time to merge then! 🎉 |
|
3 hours later and the Travis builds still haven't started 😭 |
|
Ok, Travis seems to be sulking us and don't want to start the builds. Given that Reusable still has no Unit Tests (which is a shame, I know, see #7) and that the Travis builds only lint the pod and test a carthage build — which I both tested locally using |
|
@ceyhuno @phatblat @Grubas7 Reusable 3.0 has been officially released! 🎉 Thanks a lot to all of you for contributing to this big migration! 🍾 (@ceyhuno I hope I used the right mention, I'm not sure about your Twitter handle) |
|
(@AliSoftware that's correct. It's me 👍) |
I converted the project to Swift 3 using the
masterbranch. It was not possible to rebaseswift3branch on top ofmasteras mentioned in #30 because of numerous conflicts.