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

Functional extensions for Result, DataResponse, DownloadResponse #1836

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@groue
Contributor

groue commented Dec 10, 2016

Hello,

This pull request addresses #1072.

It contains:

  • Three new Result methods: unwrap, map, and flatMap.
  • Two new DataResponse and DownloadResponse methods: map and flatMap.
  • Tests for Result
  • Inline documentation

It does not contain:

  • Tests for DataResponse and DownloadResponse, because I don't quite know where to put them
  • Documentation in README.md, because I prefer waiting for your feedback

Result.unwrap

unwrap returns a result's success value, or throws its failure error:

let result: Result<Data> = ...
try result.unwrap() // Data

Result.map

map transforms the success value of a result, while preserving its eventual failure error. Its closure argument must not throw any error:

let result: Result<Data> = ...
result.map { $0.count } // Result<Int>

Result.flatMap

flatMap transforms the success value of a result, while preserving its eventual failure error, and catches eventual transformation error:

let result: Result<Data> = ...
result.flatMap { try JSONSerialization.jsonObject(with: $0) } // Result<Any>

It is worth noting that the proposed flatMap method does not strictly follow the rules of functional programming, because its closure argument does not return another Result. Instead, its closure returns a regular value, and can throw. That's because in Swift, throwing methods are more fundamental than the Result type, and the sample code above looks like it is the right thing to do. Besides, users who use Result extensively can still use the unwrap method in their flatMap closures:

func f(_ data: Data) -> Result<Int> { ... }
let result: Result<Data> = ...
result.flatMap { try f($0).unwrap() } // Result<Int>

DataResponse and DownloadResponse

Their map and flatMap methods apply map and flatMap to their result, while preserving all other information (requests, timelines, etc.).

They make it easier to perform ad-hoc deserializations, when extending Alamofire with a full-fledged DataResponseSerializer is not worth it.

func getStuff(_ completionHandler: @escaping (DataResponse<Stuff>) -> Void)) {
    Alamofire.request("https://httpbin.org/get").responseJSON { response in
        completionHandler(response.flatMap { JSON in
            return try Stuff(with: JSON)
        })
    }
}

getStuff { response in
    print(response.request)  // original URL request
    print(response.response) // HTTP URL response
    print(response.data)     // server data
    print(response.result)   // result of response serialization

    if let stuff = response.result.value {
        print("stuff: \(stuff)")
    }
}

What do you think?

@groue

This comment has been minimized.

Show comment
Hide comment
@groue
Contributor

groue commented Dec 10, 2016

@cnoon cnoon requested review from jshier and cnoon Dec 16, 2016

@cnoon cnoon self-assigned this Dec 16, 2016

@groue

This comment has been minimized.

Show comment
Hide comment
@groue

groue Dec 20, 2016

Contributor

My introduction of flatMap deserves an update. I wrote:

flatMap transforms the success value of a result, while preserving its eventual failure error, and catches eventual transformation error:

let result: Result<Data> = ...
result.flatMap { try JSONSerialization.jsonObject(with: $0) } // Result<Any>

It is worth noting that the proposed flatMap method does not strictly follow the rules of functional programming, because its closure argument does not return another Result. Instead, its closure returns a regular value, and can throw. That's because in Swift, throwing methods are more fundamental than the Result type, and the sample code above looks like it is the right thing to do. Besides, users who use Result extensively can still use the unwrap method in their flatMap closures:

func f(_ data: Data) -> Result<Int> { ... }
let result: Result<Data> = ...
result.flatMap { try f($0).unwrap() } // Result<Int>

I remain convinced that the natural flatMap accepts a throwing closure, but I remain open to a flatMap overload that accepts a Result argument, and better follows the functional rules:

func f(_ data: Data) -> Result<Int> { ... }
let result: Result<Data> = ...
result.flatMap { f($0) } // Result<Int>

Yet since Swift has no standard Result type, an app that includes Alamofire may also include other libraries that define their own Result type. Is it a good idea to bless the Result type of Alamofire? That's an open question:

// Unavoidable
func f(_ data: Data) -> SomeLib.Result<Int> { ... }
let result: Alamofire.Result<Data> = ...
result.flatMap { f($0) } // Compiler error

Finally, I also introduced the unwrap function:

unwrap returns a result's success value, or throws its failure error:

let result: Result<Data> = ...
try result.unwrap() // Data

I guess the reciprocal wrap should be added as well, for completeness' sake:

let r = Result.wrap { try getInt() } // Result<Int>
Contributor

groue commented Dec 20, 2016

My introduction of flatMap deserves an update. I wrote:

flatMap transforms the success value of a result, while preserving its eventual failure error, and catches eventual transformation error:

let result: Result<Data> = ...
result.flatMap { try JSONSerialization.jsonObject(with: $0) } // Result<Any>

It is worth noting that the proposed flatMap method does not strictly follow the rules of functional programming, because its closure argument does not return another Result. Instead, its closure returns a regular value, and can throw. That's because in Swift, throwing methods are more fundamental than the Result type, and the sample code above looks like it is the right thing to do. Besides, users who use Result extensively can still use the unwrap method in their flatMap closures:

func f(_ data: Data) -> Result<Int> { ... }
let result: Result<Data> = ...
result.flatMap { try f($0).unwrap() } // Result<Int>

I remain convinced that the natural flatMap accepts a throwing closure, but I remain open to a flatMap overload that accepts a Result argument, and better follows the functional rules:

func f(_ data: Data) -> Result<Int> { ... }
let result: Result<Data> = ...
result.flatMap { f($0) } // Result<Int>

Yet since Swift has no standard Result type, an app that includes Alamofire may also include other libraries that define their own Result type. Is it a good idea to bless the Result type of Alamofire? That's an open question:

// Unavoidable
func f(_ data: Data) -> SomeLib.Result<Int> { ... }
let result: Alamofire.Result<Data> = ...
result.flatMap { f($0) } // Compiler error

Finally, I also introduced the unwrap function:

unwrap returns a result's success value, or throws its failure error:

let result: Result<Data> = ...
try result.unwrap() // Data

I guess the reciprocal wrap should be added as well, for completeness' sake:

let r = Result.wrap { try getInt() } // Result<Int>
@jshier

This comment has been minimized.

Show comment
Hide comment
@jshier

jshier Jan 10, 2017

Contributor

I'm researching what flatMap does to the Result types of other languages, but for now this use looks fine.

One thing I would also like to see, even if it's not strictly part of the functional paradigm, are closures which can access the Result's error without transforming them. I've defined the following extensions in my last few projects:

@discardableResult
func onSuccess(_ success: (_ value: Value) -> Void) -> Result {
    if case let .success(value) = self { success(value) }
    
    return self
}

@discardableResult
func ifSuccess(_ success: () -> Void) -> Result {
    if isSuccess { success() }
    
    return self
}

@discardableResult
func onFailure(_ failure: (_ error: Error) -> Void) -> Result {
    if case let .failure(error) = self { failure(error) }
    
    return self
}

@discardableResult
func ifFailure(_ failure: () -> Void) -> Result {
    if isFailure { failure() }
    
    return self
}

I think something similar could be helpful, perhaps in combination with .mapError and .flatMapError or something.

@cnoon Any input here?

Contributor

jshier commented Jan 10, 2017

I'm researching what flatMap does to the Result types of other languages, but for now this use looks fine.

One thing I would also like to see, even if it's not strictly part of the functional paradigm, are closures which can access the Result's error without transforming them. I've defined the following extensions in my last few projects:

@discardableResult
func onSuccess(_ success: (_ value: Value) -> Void) -> Result {
    if case let .success(value) = self { success(value) }
    
    return self
}

@discardableResult
func ifSuccess(_ success: () -> Void) -> Result {
    if isSuccess { success() }
    
    return self
}

@discardableResult
func onFailure(_ failure: (_ error: Error) -> Void) -> Result {
    if case let .failure(error) = self { failure(error) }
    
    return self
}

@discardableResult
func ifFailure(_ failure: () -> Void) -> Result {
    if isFailure { failure() }
    
    return self
}

I think something similar could be helpful, perhaps in combination with .mapError and .flatMapError or something.

@cnoon Any input here?

@groue

This comment has been minimized.

Show comment
Hide comment
@groue

groue Jan 11, 2017

Contributor

@jshier, @cnoon: The intent behind this PR is not to turn Alamofire into a functional programming flagship, but instead to enhance Alamofire with a tiny set of functional idioms that blend well into regular imperative Swift code.

The proposal is to remove some friction in custom response deserialization. Result.map/flatMap are only there to support DataResponse.map/flatMap which are the real added value of this PR. The final touch to this PR will be documentation and guidelines for simple response deserialization. This is what this PR is about:

// Look, ma! No custom extension to DataRequest!
func getStuff(_ completionHandler: @escaping (DataResponse<Stuff>) -> Void)) {
    Alamofire.request("https://httpbin.org/get").responseJSON { response in
        completionHandler(response.flatMap { json in try Stuff(json: json) })
    }
}

getStuff { response in
    print(response.request)  // original URL request
    print(response.response) // HTTP URL response
    print(response.data)     // server data
    print(response.result)   // result of response serialization

    if let stuff = response.result.value { // stuff is Stuff
        print("stuff: \(stuff)")
    }
}

Alamofire does not need those extensions. As you say above, application developers can already add a few extensions to Result and DataResponse that cover their use cases. Alamofire is extensible, and this is good.

It just happens that custom deserialization deserves a little polish, and that a tiny set of functional idioms improves it a lot, when properly explained and documented. Embedding antitypical/Result concepts and vocabulary is not my proposal. Enhancing deserialization is.

Contributor

groue commented Jan 11, 2017

@jshier, @cnoon: The intent behind this PR is not to turn Alamofire into a functional programming flagship, but instead to enhance Alamofire with a tiny set of functional idioms that blend well into regular imperative Swift code.

The proposal is to remove some friction in custom response deserialization. Result.map/flatMap are only there to support DataResponse.map/flatMap which are the real added value of this PR. The final touch to this PR will be documentation and guidelines for simple response deserialization. This is what this PR is about:

// Look, ma! No custom extension to DataRequest!
func getStuff(_ completionHandler: @escaping (DataResponse<Stuff>) -> Void)) {
    Alamofire.request("https://httpbin.org/get").responseJSON { response in
        completionHandler(response.flatMap { json in try Stuff(json: json) })
    }
}

getStuff { response in
    print(response.request)  // original URL request
    print(response.response) // HTTP URL response
    print(response.data)     // server data
    print(response.result)   // result of response serialization

    if let stuff = response.result.value { // stuff is Stuff
        print("stuff: \(stuff)")
    }
}

Alamofire does not need those extensions. As you say above, application developers can already add a few extensions to Result and DataResponse that cover their use cases. Alamofire is extensible, and this is good.

It just happens that custom deserialization deserves a little polish, and that a tiny set of functional idioms improves it a lot, when properly explained and documented. Embedding antitypical/Result concepts and vocabulary is not my proposal. Enhancing deserialization is.

@erica

This comment has been minimized.

Show comment
Hide comment
@erica

erica Jan 11, 2017

erica commented Jan 11, 2017

@jshier

This comment has been minimized.

Show comment
Hide comment
@jshier

jshier Jan 11, 2017

Contributor

My main concerns with dematerialize vs. unwrap are 1) translation between other Result types in Swift and ours, namely antitypical/Result, which already use dematerialize, and 2) supporting the symmetrical operation of materialize, which I'm not sure works well if it's just wrap. But that's fine.

We want this functionality, we just have to iron out the naming and how far we want to go. Because while it may be about better response handling for you @groue, making it easier to use Result in any scenario is a good idea.

Contributor

jshier commented Jan 11, 2017

My main concerns with dematerialize vs. unwrap are 1) translation between other Result types in Swift and ours, namely antitypical/Result, which already use dematerialize, and 2) supporting the symmetrical operation of materialize, which I'm not sure works well if it's just wrap. But that's fine.

We want this functionality, we just have to iron out the naming and how far we want to go. Because while it may be about better response handling for you @groue, making it easier to use Result in any scenario is a good idea.

@jshier

This comment has been minimized.

Show comment
Hide comment
@jshier

jshier Jan 11, 2017

Contributor

Thinking about it more, it would probably help interop if we used wrap/unwrap and let antitypical/Result use materialize/dematerialize, so I think I've just convince myself we're good there.

Contributor

jshier commented Jan 11, 2017

Thinking about it more, it would probably help interop if we used wrap/unwrap and let antitypical/Result use materialize/dematerialize, so I think I've just convince myself we're good there.

@erica

This comment has been minimized.

Show comment
Hide comment
@erica

erica Jan 11, 2017

erica commented Jan 11, 2017

@jshier

This comment has been minimized.

Show comment
Hide comment
@jshier

jshier Jan 11, 2017

Contributor

Mostly naming at this point, as we want this functionality. We're already using a Result type, so its existence is beyond question, so this is mainly about what other functionality it should have.

Contributor

jshier commented Jan 11, 2017

Mostly naming at this point, as we want this functionality. We're already using a Result type, so its existence is beyond question, so this is mainly about what other functionality it should have.

@erica

This comment has been minimized.

Show comment
Hide comment
@erica

erica Jan 11, 2017

erica commented Jan 11, 2017

@groue

This comment has been minimized.

Show comment
Hide comment
@groue

groue Jan 11, 2017

Contributor

@jshier, I'l keep unwrap and add wrap when you tell me so. Although wrap could be bike-shedded too 😉 :

// Factory method?
Result.wrap { try value() }

// Initializer?
Result(value: { try value() })
Result(wrapping: { try value() })

Swift fosters initializers, doesn't it? I'd vote for Result(value: { try ... }):

// Result<Stuff>
let result = Result(value: { try Stuff(json: json) })
result.value()      // Stuff?
try result.unwrap() // Stuff
Contributor

groue commented Jan 11, 2017

@jshier, I'l keep unwrap and add wrap when you tell me so. Although wrap could be bike-shedded too 😉 :

// Factory method?
Result.wrap { try value() }

// Initializer?
Result(value: { try value() })
Result(wrapping: { try value() })

Swift fosters initializers, doesn't it? I'd vote for Result(value: { try ... }):

// Result<Stuff>
let result = Result(value: { try Stuff(json: json) })
result.value()      // Stuff?
try result.unwrap() // Stuff
@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Jan 16, 2017

Member

Hi @groue,

My apologies for not getting back to you sooner. I wanted to make sure I had time to dig through this before giving you some direction. 😉

I think you're right on point here. The only thing I've found that I "think" is incorrect is this code sample from your original post. Wouldn't this return Int or throw rather than returning a Result<Int> from the unwrap?

func f(_ data: Data) -> Result<Int> { ... }
let result: Result<Data> = ...
result.flatMap { try f($0).unwrap() } // Result<Int>

Just making sure...

Unwrap vs Dematerialize

As for unwrap vs. dematerialize...I'm much more of a fan of unwrap. Much more straightforward. Sounds like that's where all of you have landed anyways, so yay!

Factory Method vs Initializer

As for the factory method or initializer, I'd vote initializer just like you posted in your last example.

let result = Result(value: { try Stuff(json: json) })

Steps to Getting Merged

Now there are a few things we'll still need to get into this PR to get it merged.

Result initializer

As mentioned before, let's get this added along with tests.

Tests

Could we get tests for the DataResponse and DownloadResponse? You can just throw them into new test cases in the Response.swift for now.

We're going to reorganize the test suite here at some point.

Documentation

Let's also get the documentation added to the README. I think you need to be extra careful here though in regards to threading. You're example you posted is nice and convenient, it could be a huge bottleneck for large payloads being serialized on the main thread.

func getStuff(_ completionHandler: @escaping (DataResponse<Stuff>) -> Void)) {
    Alamofire.request("https://httpbin.org/get").responseJSON { response in
        completionHandler(response.flatMap { json in try Stuff(json: json) })
    }
}

While convenience is great, bad performance due to lack of understanding of what's running where is the last thing we want. Please be careful to point out these types of considerations when putting your documentation together.

Once you get the PR updated, please rebase on top of master (just to catch up) and we'll work to get this merged as part of the Alamofire 4.4.0 release.

Thanks again for all the hard work here and again our apologies for this being as delayed as it has been. 🍻

Member

cnoon commented Jan 16, 2017

Hi @groue,

My apologies for not getting back to you sooner. I wanted to make sure I had time to dig through this before giving you some direction. 😉

I think you're right on point here. The only thing I've found that I "think" is incorrect is this code sample from your original post. Wouldn't this return Int or throw rather than returning a Result<Int> from the unwrap?

func f(_ data: Data) -> Result<Int> { ... }
let result: Result<Data> = ...
result.flatMap { try f($0).unwrap() } // Result<Int>

Just making sure...

Unwrap vs Dematerialize

As for unwrap vs. dematerialize...I'm much more of a fan of unwrap. Much more straightforward. Sounds like that's where all of you have landed anyways, so yay!

Factory Method vs Initializer

As for the factory method or initializer, I'd vote initializer just like you posted in your last example.

let result = Result(value: { try Stuff(json: json) })

Steps to Getting Merged

Now there are a few things we'll still need to get into this PR to get it merged.

Result initializer

As mentioned before, let's get this added along with tests.

Tests

Could we get tests for the DataResponse and DownloadResponse? You can just throw them into new test cases in the Response.swift for now.

We're going to reorganize the test suite here at some point.

Documentation

Let's also get the documentation added to the README. I think you need to be extra careful here though in regards to threading. You're example you posted is nice and convenient, it could be a huge bottleneck for large payloads being serialized on the main thread.

func getStuff(_ completionHandler: @escaping (DataResponse<Stuff>) -> Void)) {
    Alamofire.request("https://httpbin.org/get").responseJSON { response in
        completionHandler(response.flatMap { json in try Stuff(json: json) })
    }
}

While convenience is great, bad performance due to lack of understanding of what's running where is the last thing we want. Please be careful to point out these types of considerations when putting your documentation together.

Once you get the PR updated, please rebase on top of master (just to catch up) and we'll work to get this merged as part of the Alamofire 4.4.0 release.

Thanks again for all the hard work here and again our apologies for this being as delayed as it has been. 🍻

@groue

This comment has been minimized.

Show comment
Hide comment
@groue

groue Jan 16, 2017

Contributor

Thanks @cnoon ! For the answer, the hints, and for the warning against deserialization on the main thread, which I overlooked. Requested updates to the PR are coming 😄

Contributor

groue commented Jan 16, 2017

Thanks @cnoon ! For the answer, the hints, and for the warning against deserialization on the main thread, which I overlooked. Requested updates to the PR are coming 😄

@groue

This comment has been minimized.

Show comment
Hide comment
@groue

groue Jan 16, 2017

Contributor

@cnoon, @jshier, I've rebased this PR against the master branch, added tests for DataResponse and DownloadResponse, the Result initializer reciprocal of unwrap, and presented response mapping as an introduction to custom response serialization in the README.

I did not document the new Result methods, though! It does quite not fit in any existing section - Result is just an utility, after all. How do you think we should tell Alamofire users about these additions?

Contributor

groue commented Jan 16, 2017

@cnoon, @jshier, I've rebased this PR against the master branch, added tests for DataResponse and DownloadResponse, the Result initializer reciprocal of unwrap, and presented response mapping as an introduction to custom response serialization in the README.

I did not document the new Result methods, though! It does quite not fit in any existing section - Result is just an utility, after all. How do you think we should tell Alamofire users about these additions?

@groue

This comment has been minimized.

Show comment
Hide comment
@groue

groue Jan 17, 2017

Contributor

@cnoon I forgot your question:

The only thing I've found that I "think" is incorrect is this code sample from your original post. Wouldn't this return Int or throw rather than returning a Result from the unwrap?

func f(_ data: Data) -> Result<Int> { ... }
let result: Result<Data> = ...
result.flatMap { try f($0).unwrap() } // Result<Int>

Yes, f($0).unwrap() is Int (or throws).

And result.flatMap { try f($0).unwrap() } is Result<Int>.

Contributor

groue commented Jan 17, 2017

@cnoon I forgot your question:

The only thing I've found that I "think" is incorrect is this code sample from your original post. Wouldn't this return Int or throw rather than returning a Result from the unwrap?

func f(_ data: Data) -> Result<Int> { ... }
let result: Result<Data> = ...
result.flatMap { try f($0).unwrap() } // Result<Int>

Yes, f($0).unwrap() is Int (or throws).

And result.flatMap { try f($0).unwrap() } is Result<Int>.

cnoon added a commit that referenced this pull request Feb 26, 2017

@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Feb 26, 2017

Member

Okay @groue, I just squashed your PR down and made a few formatting and documentation tweaks and pushed it up into master in 7f8fe9c while maintaining your attribution. We can't thank you enough for all your great work here and persistence given the fact that it took us a long time to get this feature added in. I think it all paid off very well though given the final result.

These changes will ship as part of the Alamofire 4.4.0 release here shortly.

Thanks again! 🍻

Member

cnoon commented Feb 26, 2017

Okay @groue, I just squashed your PR down and made a few formatting and documentation tweaks and pushed it up into master in 7f8fe9c while maintaining your attribution. We can't thank you enough for all your great work here and persistence given the fact that it took us a long time to get this feature added in. I think it all paid off very well though given the final result.

These changes will ship as part of the Alamofire 4.4.0 release here shortly.

Thanks again! 🍻

@erica

This comment has been minimized.

Show comment
Hide comment
@erica

erica Feb 26, 2017

erica commented Feb 26, 2017

@groue

This comment has been minimized.

Show comment
Hide comment
@groue

groue Feb 26, 2017

Contributor

👍 I'm quite glad we have converged on this delicate topic! Many thanks to all, @cnoon, @jshier, @erica !!!

Contributor

groue commented Feb 26, 2017

👍 I'm quite glad we have converged on this delicate topic! Many thanks to all, @cnoon, @jshier, @erica !!!

@groue groue deleted the groue:functional branch Feb 27, 2017

looseyi added a commit to looseyi/Alamofire that referenced this pull request May 22, 2017

@groue groue referenced this pull request Oct 2, 2017

Closed

Completing #2135 #2306

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment