Thomvis / BrightFutures

Write great asynchronous code in Swift using futures and promises
MIT License
1.9k stars 184 forks source link

Swift 2.2/Xcode 7.3 #131

Closed Noobish1 closed 8 years ago

Noobish1 commented 8 years ago

I believe this is all the necessary changes for Swift 2.2/Xcode 7.3.

Thomvis commented 8 years ago

Thanks so much for the PR. Looks good. A few questions/remarks:

If you agree, can you update your PR accordingly? Thanks again!

Noobish1 commented 8 years ago

Your comments are totally valid, i've bumped the podspec to 4.0.0-beta.1.

Noobish1 commented 8 years ago

Btw, i'm not sure what you wanted to do with this:

extension NoError: Equatable { }

public func ==(lhs: NoError, rhs: NoError) -> Bool {
    return true
}
pshc commented 8 years ago

If a type T in the forest has no values, is T: Equatable sound?

Thomvis commented 8 years ago

Hmm, that code looks strange indeed. I'm not sure, but I might have written it to be able to implement equals for BrightFuturesError<NoError>. I think there are some tests that depend on being able to compare errors of that type.

Noobish1 commented 8 years ago

yea that's where it's used.

Thomvis commented 8 years ago

I think what we should do is have another overload of == that accepts two BrightFuturesErrors<NoError>s.

Noobish1 commented 8 years ago

Makes sense to me, that'd be instead of the current one right?

Noobish1 commented 8 years ago

I've removed the NoError extension and == overload and added an == overload for two BrightFuturesError operands as requested.

Thomvis commented 8 years ago

Sorry for not replying sooner. I think the implementation of the == overload should be different. Right now it would mean that .NoSuchElement is equal to InvalidationTokenInvalidated. I think it should look like this:

/// Returns `true` if `left` and `right` are both of the same case ignoring .External associated value
public func ==(lhs: BrightFuturesError<NoError>, rhs: BrightFuturesError<NoError>) -> Bool {
    switch (lhs, rhs) {
    case (.NoSuchElement, .NoSuchElement): return true
    case (.InvalidationTokenInvalidated, .InvalidationTokenInvalidated): return true
    case (.External(let lhs), .External(let rhs)): return true // this cannot happen because NoError cannot be instantiated
    default: return false 
    }
}

Can you fix this. After that, the PR is ready for merging. Thanks again!

Noobish1 commented 8 years ago

That makes sense, done.

Thomvis commented 8 years ago

Thanks so much, merged it in!