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 - Response Serializers per Subclass #1457
Conversation
cc @jshier and @kcharwood for review. |
public protocol ResponseSerializerType { | ||
/// The type of serialized object to be created by this `ResponseSerializerType`. | ||
/// The type in which all data response serializers must conform to in order to serialize a response. | ||
public protocol DataResponseSerializerType { |
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 believe the Swift 3 guidelines call for these sorts of protocols to end in Protocol
now, not Type
.
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, will change.
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.
Fixed in 2cb2782.
db619cc
to
97658eb
Compare
@cnoon Review complete. Some minor feedback, but it looks great! I'm a bit worried about the growing complexity but I think if it's hidden well we should be fine. I also couldn't come up with a better name than |
6350757
to
f02a579
Compare
Fair enough @jshier on the name...I couldn't come up with a better one either given all the tradeoffs. As for the growing complexity, I think it's all manageable. It was certainly a concern of mine as well. |
…wnload requests. Previously, all response serializers could be used on any Request which was very confusing for download and stream requests. This commit moves all the previous response serializers into extensions on DataRequest which makes them only accessible to data and upload requests. The same response serializers were then re-created to support download requests as well.
f02a579
to
66712de
Compare
All comments addressed. I'm going to move this through. Thanks for the review @jshier! Much appreciated. |
This PR locks current response serializers down to data and upload requests and creates all new response serializers for download requests.
Motivation
Prior to this PR, response serializers could be applied to any
Request
subclass. This works great for data and upload requests, but completely falls apart for download and stream requests. Response serializers should only be able to be chained ontoRequest
subclasses that match the actual serializer behavior.Solution
In order to achieve this, I had to create a few new types:
DefaultDataResponse
- Encapsulates all data associated with an unserialized data or upload requestDataResponse
- Encapsulates all data associated with a serialized data or upload requestDefaultDownloadResponse
- Encapsulates all data associated with an unserialized download requestDownloadResponse
- Encapsulates all data associated with a serialized download requestDataResponseSerializerType
- data response serialization protocolDataResponseSerializer
- generic data response serializerDownloadResponseSerializerType
- download response serialization protocolDownloadResponseSerializer
- generic download response serializerDefault Responses
The
DefaultDataResponse
andDefaultDownloadResponse
structs were created to make the unserialized forms of theresponse
APIs consistent with the serialized versions in only returning a singleresponse
parameter. The consistency is a nice addition.Data and Download Response Serializers
It would have been possible to add support to the
ResponseSerializer
andResponse
types for download requests, but it would not have been an ideal set of changes. First off, the chainableRequest
APIs would still be available to theStreamRequest
. Also, theResponse
struct would have picked updestinationURL
andresumeData
properties which would only be set when serializing aDownloadRequest
. Because of these reasons, I thought it best to make a clean split between them and tailor each set of APIs to their respective subclass.Request Serialization Extensions
When I first built out the variants between data and download response serializers, I realized there was a lot of code duplication when running all the different validation scenarios. The only real difference between them was that the download response serializer first needed to extract the data out of the
destinationURL
. Once that was done, the rest of the validation was the same between them.Because of this, I pulled the common logic out into extensions on
Request
to be shared between both theDataRequest
andDownloadRequest
extension implementations. Here's an example of the String serialization.As you can see, almost all the string validation takes place in the shared
Request. serializeResponseString
method. I used this same pattern across all the response serializers to DRY it up significantly.Common Usage
Now that we have download request response serializers, you can actually download a json blob to a file and still use a
responseJSON
serializer to extract the data. No more trying to figure out what the heck thedestinationURL
actually is!Summary
In summary, these changes should fix up all sorts of confusing problems people have had with response serializers in the past. The don't make creating your own custom serializers any easier, but I don't think they make it any harder either. They now just add the ability to make custom ones for in-memory downloads or on-disk downloads.