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

Use built in Result type on Swift 5 #1146

Merged
merged 2 commits into from May 15, 2019

Conversation

@dstranz
Copy link
Contributor

dstranz commented Mar 27, 2019

I think that it's good time to hide custom Result type declaration for Swift 5 clients and switch to built-in type. For Swift 4.2 and older it will be still visible.

@dstranz dstranz changed the title Use built in Result type on Swift 5+ Use built in Result type on Swift 5 Mar 27, 2019
@onevcat

This comment has been minimized.

Copy link
Owner

onevcat commented Mar 27, 2019

Thanks for it. It looks fine with a context of Xcode 10.2 and Swift 5 compiler. But it would not work well for Swift 4.2 compiler.

First, could you change the swift(<5) to something like swift(>=5)? The current PR would prevent Xcode 10.1 users to build the framework.

Furthermore, if I am correct, there are still several Kingfisher.Result methods (value getters or something like that) are not existing in Swift.Result. I marked them as deprecated in a version before, but by switching to Swift.Result now would probably break users' implementation and causes incompatible changes.

So I want to wait for a while now until the majority become to use Xcode 10.2. We can leave this PR opened until then.

@dstranz

This comment has been minimized.

Copy link
Contributor Author

dstranz commented Mar 27, 2019

@onevcat Take a look at this one

#if swift(>=4.3)
/// Result type already built-in
#else

it looks that "next" version number was already defined in Xcode 10.1.

@Ezra-Black

This comment has been minimized.

Copy link

Ezra-Black commented Apr 27, 2019

Curious why this hasn't been pulled yet.

@MoveUpwardsDev

This comment has been minimized.

Copy link

MoveUpwardsDev commented May 15, 2019

@onevcat I just face the issue spending 3 hours to find out why my Swift.Result is not correctly taken into account.

This PR has to be merged and new release shall be delivered to avoid the same to others.

@onevcat onevcat merged commit a07fa02 into onevcat:master May 15, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MoveUpwardsDev

This comment has been minimized.

Copy link

MoveUpwardsDev commented May 15, 2019

Thanks 💯

@MoveUpwardsDev

This comment has been minimized.

Copy link

MoveUpwardsDev commented May 16, 2019

@onevcat may i ask when you plan to release a new version to cocoapods that contains this MR ?

@onevcat

This comment has been minimized.

Copy link
Owner

onevcat commented May 16, 2019

@MoveUpwardsDev I am releasing a new version for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.