Skip to content
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

Add failure constraint to *Response #2893

Merged
merged 2 commits into from Aug 22, 2019

Conversation

@philtre
Copy link
Contributor

commented Aug 7, 2019

Goals ⚽️

Add failure constraint to *Response

This PR builds on top of #2891 & #2892
and is an attempt at breaking up #2864

@philtre philtre force-pushed the philtre:feature/doubly-generic-response branch 3 times, most recently from 30e7484 to 12a7861 Aug 7, 2019

@philtre philtre referenced this pull request Aug 9, 2019

@philtre philtre force-pushed the philtre:feature/doubly-generic-response branch from 12a7861 to 87e66cb Aug 9, 2019

@jshier
Copy link
Contributor

left a comment

Well done, but I requested a few changes.

I'm still not entirely sold on these changes. This front loads the complexity of dealing with a error type onto users who almost never need it. Perhaps if we used the simpler typealias for the public versions and APIs we could get the advantage of the double generic but the users don't need to pay the cost until they need to manipulate the error values.

Source/Response.swift Outdated Show resolved Hide resolved

/// Event called when a `DataRequest` calls a `ResponseSerializer` and creates a generic `DataResponse<Value>`.
func request<Value>(_ request: DataRequest, didParseResponse response: DataResponse<Value>)
func request<Value>(_ request: DataRequest, didParseResponse response: DataResponse<Value, Error>)

This comment has been minimized.

Copy link
@jshier

jshier Aug 11, 2019

Contributor

Even though there will only be one error type here, it's probably a good idea to make this (and the corresponding download method) generic for Failure: Error as well, just to ensure we're compatible with future changes.

This comment has been minimized.

Copy link
@philtre

philtre Aug 20, 2019

Author Contributor

Done.

Source/EventMonitor.swift Outdated Show resolved Hide resolved
@@ -24,8 +24,11 @@
import Foundation

/// `DataResponse` that always has an `Error` `Failure` type.
public typealias AFDataResponse<Success> = DataResponse<Success, Error>

This comment has been minimized.

Copy link
@jshier

jshier Aug 11, 2019

Contributor

I think we need a better name here. The AF prefix doesn't buy us anything, as it's not differentiating from Swift itself this time. Perhaps SimpleDataResponse?

This comment has been minimized.

Copy link
@philtre

philtre Aug 13, 2019

Author Contributor

I agree that it doesn't make much sense right now, but if we were to wrap all internal errors in AFError, then we could turn these typealiases into:

public typealias AFResult<Success> = Result<Success, AFError>
public typealias AFDataResponse<Success> = DataResponse<Success, AFError>
public typealias AFDownloadResponse<Success> = DataResponse<Success, AFError>

All the Response extensions in ResponseSerialization could then use these in their completion closures. For example:

extension DataRequest {

    public func response(
        queue: DispatchQueue = .main,
        completionHandler: @escaping (AFDataResponse<Data?>) -> Void
    ) -> Self { ... }

    public func responseString(
        queue: DispatchQueue = .main, 
        encoding: String.Encoding? = nil, 
        completionHandler: @escaping (AFDataResponse<String>) -> Void
    ) -> Self { ... }

    public func responseJSON(
        queue: DispatchQueue = .main, 
        options: JSONSerialization.ReadingOptions = .allowFragments, 
        completionHandler: @escaping (AFDataResponse<Any>) -> Void
    ) -> Self { ... }
}

This comment has been minimized.

Copy link
@jshier

jshier Aug 13, 2019

Contributor

I can see that, though I don't think we'd do that for AFResult, as it has a specific, separate purpose right now.

This comment has been minimized.

Copy link
@philtre

philtre Aug 20, 2019

Author Contributor

I've removed this for now.

Tests/ResponseTests.swift Outdated Show resolved Hide resolved
@@ -568,95 +570,105 @@ class ResponseMapErrorTestCase: BaseTestCase {
}

waitForExpectations(timeout: timeout, handler: nil)

if response == nil { XCTFail(); return }

This comment has been minimized.

Copy link
@jshier

jshier Aug 11, 2019

Contributor

Please do not change the structure of tests as part of this PR.

This comment has been minimized.

Copy link
@philtre

philtre Aug 20, 2019

Author Contributor

Reverted.

Tests/ResponseTests.swift Outdated Show resolved Hide resolved
Tests/ResponseTests.swift Outdated Show resolved Hide resolved
Tests/ResponseTests.swift Outdated Show resolved Hide resolved
@jshier

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2019

@philtre This could also use a rebase.

@jshier

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2019

@philtre Any chance of getting this updated and rebased soon? We're working towards RC1, so it would be good to get any final changes in soon.

@philtre philtre force-pushed the philtre:feature/doubly-generic-response branch 2 times, most recently from 107015e to 2913e6d Aug 20, 2019

@philtre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

I rebased and amended the commit to address your comments and suggestions. All the tests are passing, however I've had to increase the timeout to avoid crashes (but didn't include that in the commit).

@jshier

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Which tests were crashing for you?

@philtre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

It’s pretty random but it happens when a request takes longer than 5 seconds and the code after waitForExpectations tries to access a Response which was supposed to be set in the request completion.

@jshier

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2019

Unless there's force unwrapping, it should crash, just fail the assertions.

Davor Bauk

@philtre philtre force-pushed the philtre:feature/doubly-generic-response branch from 2913e6d to eb23621 Aug 20, 2019

@jshier
Copy link
Contributor

left a comment

A few changes in the tests, otherwise this looks good to merge. Nice work!

@@ -531,7 +531,7 @@ class ResponseMapErrorTestCase: BaseTestCase {
let urlString = "https://invalid-url-here.org/this/does/not/exist"
let expectation = self.expectation(description: "request should not succeed")

var response: DataResponse<Any>?
var response: DataResponse<Any, TestError>!

This comment has been minimized.

Copy link
@jshier

jshier Aug 21, 2019

Contributor

This should just be a normal optional, as you're using optionally below.

@@ -559,7 +559,7 @@ class ResponseMapErrorTestCase: BaseTestCase {
let urlString = "https://httpbin.org/get"
let expectation = self.expectation(description: "request should succeed")

var response: DataResponse<Data>?
var response: DataResponse<Data, TestError>!

This comment has been minimized.

Copy link
@jshier

jshier Aug 21, 2019

Contributor

This should also just be a normal optional.

// Given
let urlString = "https://httpbin.org/get"
let expectation = self.expectation(description: "request should succeed")

var response: DataResponse<Data>?
var response: DataResponse<Data, Error>!

This comment has been minimized.

Copy link
@jshier

jshier Aug 21, 2019

Contributor

This should also be a normal optional.

// Given
let urlString = "https://invalid-url-here.org/this/does/not/exist"
let expectation = self.expectation(description: "request should fail")

var response: DataResponse<Data>?
var response: DataResponse<Data, Error>!

This comment has been minimized.

Copy link
@jshier

jshier Aug 21, 2019

Contributor

This should be a normal optional.

// Given
let urlString = "https://invalid-url-here.org/this/does/not/exist"
let expectation = self.expectation(description: "request should fail")

var response: DataResponse<Data>?
var response: DataResponse<Data, Error>!

This comment has been minimized.

Copy link
@jshier

jshier Aug 21, 2019

Contributor

This should be a normal optional.

@jshier

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2019

@philtre Just a few reversions in test changes, otherwise this is ready to land.

Davor Bauk
@philtre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

I've changed the values to be regular optionals. However, I've also been experiencing the same test crashes on this branch as mentioned here:
#2897 (comment)

@jshier
jshier approved these changes Aug 22, 2019
Copy link
Contributor

left a comment

👍

@jshier jshier merged commit 755dd51 into Alamofire:master Aug 22, 2019

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@jshier

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

I'm not seeing any crashes when running the tests on master.

@jshier jshier added this to the 5.0.0.rc.1 milestone Sep 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.