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.

Sign up for GitHub

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

Add a version of map that converts thrown errors to Failures. #118

Merged
merged 6 commits into from
Dec 9, 2015
Merged

Add a version of map that converts thrown errors to Failures. #118

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e2fa2bb
Add a version of `map` that converts thrown errors to `Failure`s.
Nov 6, 2015
1e277d8
Favor `fatalError` over `as!`.
Nov 6, 2015
dee4350
Add space between parameter name and type.
Dec 2, 2015
329b819
Merge branch 'master' into tryMap
bwhiteley Dec 2, 2015
855a5d7
Rename `map` for `transform`s that throw to `tryMap`
bwhiteley Dec 2, 2015
2e5749a
In `tryMap`, switch to `as!` when converting error.
bwhiteley Dec 5, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions Result/Result.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,4 +201,14 @@ public func >>- <T, U, Error> (result: Result<T, Error>, @noescape transform: T
}


// MARK: - ErrorTypeConvertible conformance

/// Make NSError conform to ErrorTypeConvertible
extension NSError: ErrorTypeConvertible {
public static func errorFromErrorType(error: ErrorType) -> NSError {
return error as NSError
}
}


import Foundation
24 changes: 24 additions & 0 deletions Result/ResultType.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,30 @@ public extension ResultType {
}
}

/// Protocol used to constrain `tryMap` to `Result`s with compatible `Error`s.
public protocol ErrorTypeConvertible: ErrorType {
typealias ConvertibleType = Self
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? Maybe just use Self?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is necessary because NSError is not final. From what I've seen, this is a common pattern.

static func errorFromErrorType(error: ErrorType) -> ConvertibleType
}

public extension ResultType where Error: ErrorTypeConvertible {

/// Returns the result of applying `transform` to `Success`es’ values, or wrapping thrown errors.
public func tryMap<U>(@noescape transform: Value throws -> U) -> Result<U, Error> {
return flatMap { value in
do {
return .Success(try transform(value))
}
catch {
guard let convertedError = Error.errorFromErrorType(error) as? Error else {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused about this, if Error conforms to ErrorTypeConvertible, this is always going to be non-nil and of type Error, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You would think so, but

ResultType.swift:82:58: error: 'Self.Error.ConvertibleType' is not convertible to 'Self.Error'; did you mean to use 'as!' to force downcast?
                                let convertedError = Error.errorFromErrorType(error) as Error
                                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~
                                                                                     as!

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like a bug.
What if you remove ConvertibleType and change ErrorTypeConvertible:

public protocol ErrorTypeConvertible: ErrorType {
    static func errorFromErrorType(error: ErrorType) -> Self
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does permit the use of a plain as in tryMap, but then I can't make NSError conform to ErrorTypeConvertible, presumably because it's not final.

I boiled it down to a sample that you can tinker with in a Playground.

import Foundation

// For types that can be created from an arbitrary ErrorType
public protocol ErrorTypeConvertible: ErrorType {
    typealias ConvertibleType = Self
    static func errorFromErrorType(error: ErrorType) -> ConvertibleType
}

protocol MyType {
    typealias Error: ErrorType
    var error: Error { get }
}

extension MyType where Error: ErrorTypeConvertible {
    func foo() -> Error {
        return Error.errorFromErrorType(error) as! Error // ** Why does this require `!`? **
    }
}

extension NSError: ErrorTypeConvertible {
    public static func errorFromErrorType(error: ErrorType) -> NSError {
        return error as NSError
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably related to https://twitter.com/jckarter/status/672931114944696321.

Given that the cast can never fail I'd just use as! and add a comment to improve this later on.

fatalError("Unable to convert error type '\(error.dynamicType)' to type '\(Error.self)'")
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool

}
return .Failure(convertedError)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this actually behaves like flatMap (but with a restricted parameter type), calling it map seems a bit confusing 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, and would have preferred to call it flatMap. However, that resulted in "ambiguous use..." errors. Do you have a recommendation?

Copy link
Contributor

Choose a reason for hiding this comment

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

/cc @antitypical/result

Copy link
Member

Choose a reason for hiding this comment

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

Spitballing, but: Is there maybe a way to rewrite the current flatMap with rethrows in a way that covers both cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. The return type of the transform is quite different, and part of the reason for this PR is to be able to pass in functions that are not aware of Result.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I don’t think this is precisely map or flatMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally called it tryMap (PR #114), but tried to roll it into flatMap or map based on feedback in that PR. How are we feeling about tryMap?

Copy link
Member

Choose a reason for hiding this comment

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

👍 on tryMap

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that was my bad, my apologies. I’m 👍 on the name tryMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed it to tryMap. Any other concerns?

}
}
}
}

// MARK: - Operators

infix operator &&& {
Expand Down
12 changes: 11 additions & 1 deletion ResultTests/ResultTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,16 @@ final class ResultTests: XCTestCase {
XCTAssertNil(result.error)
}

func testTryMapProducesSuccess() {
let result = success.tryMap(tryIsSuccess)
XCTAssert(result == success)
}

func testTryMapProducesFailure() {
let result = Result<String, NSError>.Success("fail").tryMap(tryIsSuccess)
XCTAssert(result == failure)
}

// MARK: Operators

func testConjunctionOperator() {
Expand Down Expand Up @@ -132,7 +142,7 @@ func attempt<T>(value: T, succeed: Bool, error: NSErrorPointer) -> T? {
}

func tryIsSuccess(text: String?) throws -> String {
guard let text = text else {
guard let text = text where text == "success" else {
throw error
}

Expand Down