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 AF*Response, AFResult types, update tests to use AFError #2921

Merged
merged 5 commits into from Sep 3, 2019

Conversation

@jshier
Copy link
Contributor

commented Aug 28, 2019

Goals ⚽️

This PR adds AFDataResponse and AFDownloadResponse, which don't require an explicit Failure type, just defaulting to AFError. Additionally, AFResult has also been updated to expect an AFError Failure type as well, as many users have already transitioned to Swift 5's Result. These types are used to simplify some of the response APIs.

Implementation Details 🚧

This PR also refactors a lot of tests to take advantage of the fact that AFError is now used directly in most cases and to be more explicit around different errors wrapped in AFError.

Testing Details 🔍

No new tests, just refactors.

@jshier jshier requested a review from cnoon Aug 28, 2019

@jshier jshier added this to the 5.0.0.rc.1 milestone Aug 28, 2019

@jshier jshier force-pushed the feature/easier-double-generics branch from cbde75b to 00c9d58 Aug 29, 2019

@jshier jshier force-pushed the feature/easier-double-generics branch from 00c9d58 to b1ee828 Aug 29, 2019

@cnoon
Copy link
Member

left a comment

Change looks good overall @jshier. I have some concerns about the naming conventions which I called out and a few newline nits.

Source/NetworkReachabilityManager.swift Show resolved Hide resolved
expectation.fulfill()
}
let expectation = self.expectation(description: "Download should complete but fail to move file")
var response: DownloadResponse<URL?, AFError>?

This comment has been minimized.

Copy link
@cnoon

cnoon Sep 3, 2019

Member

You could probably collapse these as well if you wanted.

This comment has been minimized.

Copy link
@jshier

jshier Sep 3, 2019

Author Contributor

Collapse what?

This comment has been minimized.

Copy link
@cnoon

cnoon Sep 3, 2019

Member

Sorry, DownloadResponse -> AFDownloadResponse.

This comment has been minimized.

Copy link
@jshier

jshier Sep 3, 2019

Author Contributor

We could do this for all uses of the Response types. For now, we can keep them consistently the base type. If we want to use the AF types, we can make the changes all at once I think.

Tests/ParameterEncoderTests.swift Show resolved Hide resolved
Tests/ResponseTests.swift Show resolved Hide resolved
Tests/TLSEvaluationTests.swift Show resolved Hide resolved
@jshier

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@cnoon Updated.

@cnoon
cnoon approved these changes Sep 3, 2019
Copy link
Member

left a comment

One comment remaining about collapsing some DownloadResponse types into AFDownloadResponse if you want to change it. Otherwise, it looks good to me. 👌🏻

@jshier jshier merged commit 6a75f4f into master Sep 3, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@jshier jshier deleted the feature/easier-double-generics branch Sep 3, 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.