Repository navigation
Conversation
|
@ikesyo This PR is ready for review! Major changes are listed below:
|
There was a problem hiding this comment.
It occurs to me that wrapping this error in APIKitError may be nice, like APIKitError.SerializationError(URLEncodedSerialization.Error).
|
I've reviewed almost all the changes. 💖 for the general direction and protocol extensions! But IMHO we should continue considering about error definitions/designs as above. |
|
Associating details to the error is very nice. I'll work on it! |
|
Now that you are planning a "rethink" for Swift 2, is there still a need for the Result dependency? with |
|
@raulriera IMO asynchronous operations like |
@ikesyo Thank you for this nice feedback. I found the 3rd idea: public class API {
public enum Error {
case RequestError(API.RequestError)
case ResponseError(API.ResponseError)
case ConnectionError(NSError)
}
public enum RequestError {
case InvalidBaseURL(NSURL) // Request.baseurl()
case ConfigurationError(ErrorType) // Request.configureURLRequest()
case BodySerializationError(ErrorType) // RequestBodyBuilder.buildBodyFromObject()
case FailedToCreateURLSessionTask // NSURLSession.dataTaskWithRequest()
}
public enum ResponseError {
case ApplicationError(ErrorType) // errorFromObject()
case BodyDeserializationError(ErrorType) // ResponseBodyParser.parseData()
case NotHTTPURLResponse(NSURLResponse) // failed to cast URLResponse to NSHTTPURLResponse
}
...
|
|
@raulriera Yes, sendRequest(request) { result in
do {
let response = try result.dematerialize()
print(response)
} catch {
print(error)
}
} |
|
You can get away by wrapping it in a closure themselves https://gist.github.com/amberstar/7861aee759b5d363d316, but I agree that it reads a bit uglier with that approach :/ |
|
Yes, I already knew that approach. Though that seems very hard to use or to understand for the library users, as you said. |
|
I pushed large commit 39d9ab2. This contains 3 large change:
First, I chose flat enum
public enum APIError: ErrorType {
case ConnectionError(NSError)
case InvalidBaseURL(NSURL)
case ConfigurationError(ErrorType)
case RequestBodySerializationError(ErrorType)
case FailedToCreateURLSessionTask
case UnacceptableStatusCode(Int, ErrorType)
case ResponseBodyDeserializationError(ErrorType)
case InvalidResponseStructure(AnyObject)
case NotHTTPURLResponse(NSURLResponse?)
}Next, I noticed it is not clear what kind of error should be thrown in |
There was a problem hiding this comment.
Why returning NSURLSettionTask? is removed? I think this make it difficult a bit to just cancel the created request/task afterward.
There was a problem hiding this comment.
Oh, it is just a mistake while refactoring. Thank you for catching!
|
I definitely like the changes made in 39d9ab2. ✨ I left just some minor notes about request canceling/documentations. Let's gonna merge this after you address them. 😉 |
|
And also merging |
|
Thank you for reviewing! I'll merge this soon. |
Swift 2 is coming! I think redesigning APIKit for Swift 2 is necessary to take full advantage of it.
Reuqestprotocol more flexilble.ErrorTypeis useful to express errors in APIKit.do-try-catchintroduced easier error handling model (if process is synchronous).Here are tasks:
Request.ErrorType.do-try-catchin synchronous error handling.Any comments are welcome.