Redesign for Swift 2

[ishkawa]

Swift 2 is coming! I think redesigning APIKit for Swift 2 is necessary to take full advantage of it.

• protocol extension makes Reuqest protocol more flexilble.
• ErrorType is useful to express errors in APIKit.
• do-try-catch introduced easier error handling model (if process is synchronous).

Here are tasks:

• Move default configuration of request to extension of Request.
• Detailed and comprehensive errors using ErrorType.
• Adopt do-try-catch in synchronous error handling.

Any comments are welcome.

[ishkawa]

@ikesyo This PR is ready for review! Major changes are listed below:

• Change steps to create NSURLRequest from an instance of Request.
  • Add baseURL, method, path and parameters properties to Request protocol.
  • Replace URLRequest property with buildURLRequest() method, which is an internal method.
  • Add configureURLRequest(_:) to add arbitrary configurations to NSMutableURLRequest.
• Move configurations of serializing HTTP body to Request protocol.
  • Move requestBodyBuilder in API class to Request protocol.
  • Move responseBodyParser in API class to Request protocol.
  • Change parameter of sendRequest(_:URLSession:resposeBodyParser:handler:) to sendRequest(_:URLSession:handler:)
• APIKitError is introduced.
• Method is now HTTPMethod to avoid conflict.
• Refactor using throw-catch error handling model.

[ikesyo]

It occurs to me that wrapping this error in APIKitError may be nice, like APIKitError.SerializationError(URLEncodedSerialization.Error).

[ikesyo]

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.

[ishkawa]

Associating details to the error is very nice. I'll work on it!

[raulriera]

Now that you are planning a "rethink" for Swift 2, is there still a need for the Result dependency? with throw/s the dependency seems a bit redundant

[ikesyo]

@raulriera IMO asynchronous operations like sendRequest() with callbacks does not work well with try/catch/throws. So Result type is still needed here. As seen in the diff, Result is now used only for the handler of sendRequest().

[ishkawa]

.UnexpectedResponse lacks meaningful info about what is unexpected. The caller can't detect the root cause of that error without debugging and stack traces. So I think there are two points we could address here:

1. Whether notifying "unexpected response" from errorFromObject() is implementor's responsibility or not. They may have a following error type and throw it in errorFromObject():

enum ErrorResponse: ErrorType {
    case InvalidRawResponse(object: AnyObject, URLResponse: NSHTTPURLResponse)
    case BadRequest(message: String)
}

In this scenario, only APIKitError.ResponseError(ErrorType) will be passed to handlers.
2. Add an/some associated values to .UnexpectedResponse. Is it ErrorType or NSURLResponse? or (object: AnyObject, URLResponse: NSHTTPURLResponse)?

How do you think about my thought above?

@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
    }

    ...

[ishkawa]

@raulriera Yes, Result is still necessary in asynchronous code like sendRequest() because it is impossible to catch an error from a different scope. FYI, you can use try-catch model with Result (next version) like this:

sendRequest(request) { result in
    do {
        let response = try result.dematerialize()
        print(response)
    } catch {
        print(error)
    }
}

[raulriera]

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 :/

[ikesyo]

Yes, I already knew that approach. Though that seems very hard to use or to understand for the library users, as you said.

[ishkawa]

I pushed large commit 39d9ab2. This contains 3 large change:

• Introduce detailed error for API.
• Stop throwing errors from responseFromObject().
• Refactor sendRequest() to keep type info about an error.

First, I chose flat enum APIError for the error type of API for 2 reasons:

• Easy to choose the error to handle from many variations.
• Detailed enough.

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 responseFromObject(). If an user want to throw a some error there, to define custom ErrorType is required. I think it is too awkward, so I change responseFromObject() throws -> Response to responseFromObject() -> Response?. With the new version of this method, an user doesn't have to define custom ErrorType to notify error. And if it returns nil, the error in handler of sendRequest will be case InvalidResponseStructure(AnyObject).

[ikesyo]

Why returning NSURLSettionTask? is removed? I think this make it difficult a bit to just cancel the created request/task afterward.

[ishkawa]

Oh, it is just a mistake while refactoring. Thank you for catching!

[ikesyo]

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. 😉

[ikesyo]

And also merging master into this is needed to resolve conflicts?

[ishkawa]

Thank you for reviewing! I'll merge this soon.