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

Mark RequestInterceptor closure callback arguments escaping #2747

Merged

Conversation

Projects
None yet
3 participants
@cysp
Copy link
Contributor

commented Mar 15, 2019

Goals ⚽️

AdaptHandler and RetryHandler are defined as closure equivalents of the RequestAdapter and RequestRetrier protocols but their completion arguments are not marked @escaping as the equivalents in the protocols are, the upshot being that implementations using the closure interfaces can not be made asynchronous.

Implementation Details 🚧

Users of the concrete Adapter, Retrier and Interceptor classes will now be able to call their completion closures asynchronously, just as implementers of the equivalent protocols can.

Testing Details 🔍

Added tests for using the concrete closure-based implementations asynchronously. 😃

@cnoon

cnoon approved these changes Mar 15, 2019

Copy link
Member

left a comment

Looks good to me @cysp. Thank you for putting this together along with writing the tests! 🍻

@jshier
Copy link
Contributor

left a comment

Looks good, but please use our standard timeout when waiting for expectations.

completesExpectation.fulfill()
}

waitForExpectations(timeout: 0.01)

This comment has been minimized.

Copy link
@jshier

jshier Mar 17, 2019

Contributor

Please use our default timeout. It should be available as timeout.

This comment has been minimized.

Copy link
@cysp

cysp Mar 17, 2019

Author Contributor

Oh, sure thing. 🙂

Show resolved Hide resolved Tests/RequestInterceptorTests.swift Outdated
Show resolved Hide resolved Tests/RequestInterceptorTests.swift Outdated
Show resolved Hide resolved Tests/RequestInterceptorTests.swift Outdated

@cysp cysp force-pushed the ScentreGroup:bugfix/requestinterceptor-escaping branch from 303a8b6 to 107f72c Mar 17, 2019

@cysp

This comment has been minimized.

Copy link
Contributor Author

commented Mar 17, 2019

The red on Travis looks spurious (in one of the jobs, a request that expected a 4xx status instead received a 5xx one).

@cysp

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Ping @jshier ? 🙂

@cysp cysp force-pushed the ScentreGroup:bugfix/requestinterceptor-escaping branch 5 times, most recently from 135ae21 to 6b7ef85 Mar 22, 2019

@cysp

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2019

I've tried pushing no-code-change commit changes a few times but different asynchronous tests keep spuriously failing.

@cysp cysp force-pushed the ScentreGroup:bugfix/requestinterceptor-escaping branch from 6b7ef85 to 107f72c Mar 24, 2019

@jshier

jshier approved these changes Mar 24, 2019

@jshier

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

Thanks!

@jshier jshier merged commit 416825c into Alamofire:master Mar 24, 2019

1 check failed

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

@cysp cysp deleted the ScentreGroup:bugfix/requestinterceptor-escaping branch Mar 24, 2019

@cnoon cnoon added this to the 5.0.0-beta.4 milestone Mar 29, 2019

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