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

Resolve the deferred returned by send_multiResponse and send_multiRes… #239

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

spencermaxfield
Copy link

Resolve the deferred returned by send_multiResponse and send_multiResponse_ex when a handler is provided and the final response has been handled.

This makes using send_multiResponse and send_multiResponse_ex easier if the caller wants to yield until all responses have been handled. With existing behavior where the returned deferred is never fired and you want to yield, you're forced to create and track a separate Deferred, provide it to the handler passed to the called send.. method, and both fire that deferred and return True 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:

  • I have updated the release notes at docs/source/NEWS.rst
  • I have updated the automated tests.
  • All tests pass on your local dev environment. See CONTRIBUTING.rst.

…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.
Copy link
Author

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.
Copy link
Author

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)
Copy link
Author

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)
Copy link
Author

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):
Copy link
Author

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.

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.

None yet

1 participant