Sitelet https://github.com/ishkawa/APIKit/pull/49
Skip to content

Redesign for Swift 2 - #49

Merged
ishkawa merged 46 commits into
masterfrom
swift2
Jun 30, 2015
Merged

ishkawa merged 46 commits into
masterfrom
swift2

Conversation

@ishkawa

@ishkawa ishkawa commented Jun 11, 2015

Copy link
Copy Markdown
Owner

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 ishkawa changed the title [WIP] Resign for Swift 2 [WIP] Redesign for Swift 2 Jun 11, 2015
@ishkawa

ishkawa commented Jun 20, 2015

Copy link
Copy Markdown
Owner Author

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ikesyo

ikesyo commented Jun 24, 2015

Copy link
Copy Markdown
Collaborator

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

ishkawa commented Jun 24, 2015

Copy link
Copy Markdown
Owner Author

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

@raulriera

Copy link
Copy Markdown

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

ikesyo commented Jun 24, 2015

Copy link
Copy Markdown
Collaborator

@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

ishkawa commented Jun 24, 2015

Copy link
Copy Markdown
Owner Author

.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

ishkawa commented Jun 24, 2015

Copy link
Copy Markdown
Owner Author

@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

Copy link
Copy Markdown

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

ikesyo commented Jun 24, 2015

Copy link
Copy Markdown
Collaborator

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

@ishkawa

ishkawa commented Jun 25, 2015

Copy link
Copy Markdown
Owner Author

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

Comment thread APIKit/API.swift Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ikesyo

ikesyo commented Jun 28, 2015

Copy link
Copy Markdown
Collaborator

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

ikesyo commented Jun 28, 2015

Copy link
Copy Markdown
Collaborator

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

@ishkawa

ishkawa commented Jun 30, 2015

Copy link
Copy Markdown
Owner Author

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

ishkawa added a commit that referenced this pull request Jun 30, 2015
@ishkawa
ishkawa merged commit fd74e39 into master Jun 30, 2015
@ishkawa
ishkawa deleted the swift2 branch June 30, 2015 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants