Skip to content
This repository has been archived by the owner on Jul 3, 2022. It is now read-only.

Swift 5 #206

Merged
merged 20 commits into from
Apr 11, 2019
Merged

Swift 5 #206

merged 20 commits into from
Apr 11, 2019

Conversation

kimdv
Copy link
Collaborator

@kimdv kimdv commented Mar 26, 2019

Hi!

I started porting to Swift 5 with the new Swift Result and it resolves #205

Had some problems with the generics in Swift, so needed to copy the some classes from the old Result framework.

It would be nice if we somehow can remove those.

Thanks! 🚀

@phimage
Copy link
Collaborator

phimage commented Mar 27, 2019

  • Carfile and Cartfile.resolved could be removed
  • Package.swift could be edited

@Thomvis
Copy link
Owner

Thomvis commented Mar 27, 2019

Thanks for the PR. I'll have a look at it tomorrow.

@mathebox
Copy link
Contributor

I think NoError can also be replaced by Never.
Alternatively, you could define a typealias to avoid breaking changes.

https://github.com/apple/swift-evolution/blob/master/proposals/0215-conform-never-to-hashable-and-equatable.md

@Thomvis
Copy link
Owner

Thomvis commented Mar 28, 2019

I've been adding some changes on top of this PR, which can be found here: https://github.com/Thomvis/BrightFutures/tree/kimdv-kimdv/swift-5.

@mathebox I agree with your suggestion to use Never. Breaking changes are fine by me.

I think ResultProtocol is still needed, so we can keep that.

@kimdv kimdv marked this pull request as ready for review March 28, 2019 13:43
@kimdv
Copy link
Collaborator Author

kimdv commented Mar 28, 2019

Rebased on solved conflicts

@Thomvis
Copy link
Owner

Thomvis commented Mar 28, 2019

Thanks! A few comments:

  • Can you revert the @testable imports back to regular imports? Regular imports are stricter and should suffice.
  • CI is failing, I think you at least need to update the travis configuration to use the 12.2 simulators
  • Can you make sure the Swift 5 migrator has run for all framework & test targets (this should also update the Swift language version to 5 for all targets)

@Thomvis
Copy link
Owner

Thomvis commented Apr 4, 2019

Thanks for your work! I think this could be released as a first beta of 8.0.

There's one thing I'm trying to figure out: can we make this work without adding new public API to the Result type. Users of BrightFutures will now, for example, all have a result property on their Result instances. I think that's suboptimal. Not sure if we can circumvent this. Any ideas? If not, we should probably move ahead and create a release.

@kimdv
Copy link
Collaborator Author

kimdv commented Apr 4, 2019

So you think we should make ResultProtocol internal?

I don't think it is possible because it is used on other public API's

@Thomvis
Copy link
Owner

Thomvis commented Apr 11, 2019

Since ReactiveCocoa/ReactiveSwift#702 is doing something very similar to what we're doing, I believe we're good and I'll move forward with the release.

@Thomvis Thomvis merged commit 02cbbcc into Thomvis:master Apr 11, 2019
@Thomvis
Copy link
Owner

Thomvis commented Apr 11, 2019

Thanks again for your work. 8.0.0 has been released! 🚀

I've invited you as a collaborator if you wish to contribute in the future.

@kimdv kimdv deleted the kimdv/swift-5 branch April 11, 2019 10:54
@kimdv
Copy link
Collaborator Author

kimdv commented Apr 11, 2019

Thanks a lot @Thomvis ! I have accepted 🚀

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.

Support Swift 5
4 participants