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

Feature - Opening up Default Response Initializers #1865

Merged
merged 2 commits into from Jan 14, 2017

Conversation

Projects
None yet
3 participants
@cnoon
Member

cnoon commented Dec 20, 2016

This PR modifies the DefaultDataResponse and DefaultDownloadResponse initializers to be public instead of internal. This allows you to rebuild the responses if necessary in client code to do things like wrap the error in your own custom error or modify the server data or possibly inject a missing header, etc. The main use case is to wrap the error.

This PR also adds convenience properties to the DataResponse and DownloadResponse structs for accessing the value and error in the result. Both of these computed properties are merely for convenience.

cnoon added some commits Dec 19, 2016

Made initializers for both default responses public and added metrics…
… parameter.

This change allows default responses to be rebuilt in client APIs as necessary. Certain use cases would be wrapping up errors in an external error type as well as modifying the data payload for a downstream response handler.

@cnoon cnoon requested a review from jshier Dec 20, 2016

@cnoon cnoon added this to the 4.3.0 milestone Dec 20, 2016

@jshier

jshier approved these changes Dec 21, 2016

👍

@groue

This comment has been minimized.

Show comment
Hide comment
@groue

groue Jan 10, 2017

Contributor

This allows you to rebuild the responses if necessary in client code to do things like wrap the error in your own custom error or modify the server data or possibly inject a missing header, etc. The main use case is to wrap the error.

Side note: #1836 also addresses error handling, maybe in a superior way: DataResponse.map and DataResponse.flatMap do recreate new responses that wrap a new result or a new error, but without exposing public initializers:

Alamofire.request("https://httpbin.org/get").responseJSON { response in
    // Here response is DataResponse<[String:Any]>,
    // and stuffResponse is DataResponse<Stuff>:
    let stuffResponse = response.flatMap { json in try Stuff(json: json) }
}

Besides, preserving responses' metrics is only possible with internal APIs (see https://github.com/Alamofire/Alamofire/pull/1836/files#diff-0532e61b339db1c06287f9110afe7dfcR147).

It's not that I want to push my #1836 at all cost. I just want to say that it addresses response transformations in a way that prevents the complex initializers from bubbling up into the public API.

Contributor

groue commented Jan 10, 2017

This allows you to rebuild the responses if necessary in client code to do things like wrap the error in your own custom error or modify the server data or possibly inject a missing header, etc. The main use case is to wrap the error.

Side note: #1836 also addresses error handling, maybe in a superior way: DataResponse.map and DataResponse.flatMap do recreate new responses that wrap a new result or a new error, but without exposing public initializers:

Alamofire.request("https://httpbin.org/get").responseJSON { response in
    // Here response is DataResponse<[String:Any]>,
    // and stuffResponse is DataResponse<Stuff>:
    let stuffResponse = response.flatMap { json in try Stuff(json: json) }
}

Besides, preserving responses' metrics is only possible with internal APIs (see https://github.com/Alamofire/Alamofire/pull/1836/files#diff-0532e61b339db1c06287f9110afe7dfcR147).

It's not that I want to push my #1836 at all cost. I just want to say that it addresses response transformations in a way that prevents the complex initializers from bubbling up into the public API.

@jshier

This comment has been minimized.

Show comment
Hide comment
@jshier

jshier Jan 10, 2017

Contributor

@groue I think the functional approach your PR creates is great, and I'm sure many developers will be comfortable there, but there is an argument for the straightforward use of an initializer. Depending on whatever else there doing, one approach may be more readable than the other or fit better with their other code.

Contributor

jshier commented Jan 10, 2017

@groue I think the functional approach your PR creates is great, and I'm sure many developers will be comfortable there, but there is an argument for the straightforward use of an initializer. Depending on whatever else there doing, one approach may be more readable than the other or fit better with their other code.

@groue

This comment has been minimized.

Show comment
Hide comment
@groue

groue Jan 10, 2017

Contributor

@jshier That's perfectly clear. I just saw an opportunity to explain some uses of the functional approach which can be overlooked - since #1836 had no feedback whatsoever. Now I'm not a functional guru/evangelist/lobbyist - I'm a regular imperative developer, and certainly don't want to force abstract functional cathedrals down into anyone's throat. #1836 is actually pretty narrow.

Now the use cases exposed by @cnoon are not straightforward: injecting headers, modifying server data, etc. That looks pretty advanced to me.

On the other side, map/flatMap are a concise syntax for a very common use case, which is to turn server data into application types, while preserving errors and response metadata. The map/flatMap idioms are, I think, simpler than extensions that custom response serialization requires today. And they blend naturally in the map/flatMap landscape that Swift already brings with optionals and collections.

Maybe give a look at #1836 some day? You may eventually find it useful.

Contributor

groue commented Jan 10, 2017

@jshier That's perfectly clear. I just saw an opportunity to explain some uses of the functional approach which can be overlooked - since #1836 had no feedback whatsoever. Now I'm not a functional guru/evangelist/lobbyist - I'm a regular imperative developer, and certainly don't want to force abstract functional cathedrals down into anyone's throat. #1836 is actually pretty narrow.

Now the use cases exposed by @cnoon are not straightforward: injecting headers, modifying server data, etc. That looks pretty advanced to me.

On the other side, map/flatMap are a concise syntax for a very common use case, which is to turn server data into application types, while preserving errors and response metadata. The map/flatMap idioms are, I think, simpler than extensions that custom response serialization requires today. And they blend naturally in the map/flatMap landscape that Swift already brings with optionals and collections.

Maybe give a look at #1836 some day? You may eventually find it useful.

@cnoon

This comment has been minimized.

Show comment
Hide comment
@cnoon

cnoon Jan 14, 2017

Member

Thanks for the feedback @groue! I agree with @jshier that both are useful. Apologies for your PR being left out there without feedback. We got pretty busy with the holidays and haven't been able to keep up with all the great work coming in.

Member

cnoon commented Jan 14, 2017

Thanks for the feedback @groue! I agree with @jshier that both are useful. Apologies for your PR being left out there without feedback. We got pretty busy with the holidays and haven't been able to keep up with all the great work coming in.

@cnoon cnoon merged commit 8525217 into master Jan 14, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@cnoon cnoon deleted the feature/opening-up-default-response-initializers branch Jan 14, 2017

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