-
Notifications
You must be signed in to change notification settings - Fork 53
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
Resolve the deferred returned by send_multiResponse and send_multiRes… #239
base: master
Are you sure you want to change the base?
Resolve the deferred returned by send_multiResponse and send_multiRes… #239
Conversation
…ponse_ex when a handler is provided and the final response has been handled
@@ -5,6 +5,8 @@ Changelog | |||
------------------- | |||
|
|||
- Dropped support for Python 3.5 | |||
- The Deferred returned from send_multiResponse and send_multiResponse_ex is | |||
now fired after the final response is handled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to suggestions on better wording here. I wasn't sure how to say this differently without getting too technical for release notes.
|
||
If `handler` is not provided, the Deferred returned by this | ||
function will fire with the final LDAP response. | ||
function will fire with the first LDAP response. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was wrong, as is evidenced by the updated test_send_multiResponse_with_handler
test
del self.onwire[msg.id] | ||
else: | ||
if handler(msg.value, *args, **kwargs): | ||
d.callback(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The functional changes are these two d.callback
lines. This lets callers of send_multiResponse
and send_multiResponse_ex
yield on those methods if they want to yield until all responses have been handled.
@@ -203,12 +203,16 @@ def collect_result_(result): | |||
) | |||
resp_bytestring = response.toWire() | |||
client.dataReceived(resp_bytestring) | |||
self.assertEqual(response.value, results[0]) | |||
self.assertFalse(d.called) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making sure the deferred is not fired until the multi-response handler returns True
@@ -229,6 +233,45 @@ def test_send_multiResponse_ex(self): | |||
client.dataReceived(resp_bytestring) | |||
self.assertEqual((response.value, response.controls), self.successResultOf(d)) | |||
|
|||
def test_send_multiResponse_ex_with_handler(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was no test covering send_multiResponse_ex
when that method was passed a handler
.
Resolve the deferred returned by
send_multiResponse
andsend_multiResponse_ex
when a handler is provided and the final response has been handled.This makes using
send_multiResponse
andsend_multiResponse_ex
easier if the caller wants toyield
until all responses have been handled. With existing behavior where the returned deferred is never fired and you want toyield
, you're forced to create and track a separateDeferred
, provide it to thehandler
passed to the calledsend..
method, and both fire that deferred and returnTrue
after the last response is handled.This change will not affect any existing callers of the methods prior behavior was that the returned deferred did nothing
Contributor Checklist:
docs/source/NEWS.rst
CONTRIBUTING.rst
.