-
Notifications
You must be signed in to change notification settings - Fork 227
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
Add a version of map
that converts thrown errors to Failure
s.
#118
Conversation
do { | ||
return .Success(try transform(value)) | ||
} catch { | ||
return .Failure(Error.errorFromErrorType(error) as! Error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This as! Error
is scary.
Can you do instead
do {
...
} catch error as Error {
return .Failure(Error.errorFromErrorType(error)
} catch error {
// what do we do here? `fatalError` I guess? (which would be equivalent to this implementation)
}
} | ||
catch { | ||
guard let convertedError = Error.errorFromErrorType(error) as? Error else { | ||
fatalError("Unable to convert error type '\(error.dynamicType)' to type '\(Error.self)'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool
What's the verdict on this PR? |
guard let convertedError = Error.errorFromErrorType(error) as? Error else { | ||
fatalError("Unable to convert error type '\(error.dynamicType)' to type '\(Error.self)'") | ||
} | ||
return .Failure(convertedError) |
There was a problem hiding this comment.
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 😕
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @antitypical/result
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 on tryMap
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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?
👍 from me. @NachoSoto? |
return .Success(try transform(value)) | ||
} | ||
catch { | ||
guard let convertedError = Error.errorFromErrorType(error) as? Error else { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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
}
}
There was a problem hiding this comment.
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.
👍 thanks @bwhiteley! this looks good now :) |
Add a version of `map` that converts thrown errors to `Failure`s.
Add a variant of
map
that handlestransform
s that throw errors and converts errors toFailure
s.This
map
is constrained toResult
s withError
types that can be constructed from arbitraryErrorType
s.