agoragames / haigha

AMQP Python client
BSD 3-Clause "New" or "Revised" License
161 stars 41 forks source link

Implemented support and unit tests for handling basic.return #73

Closed vitaly-krugl closed 9 years ago

vitaly-krugl commented 9 years ago

Implemented support and unit tests for handling basic.return, including Message.return_info and BasicClass.set_return_listener()

vitaly-krugl commented 9 years ago

@awestendorf, this implements proper handling of basic.return, which addresses the root cause of issue #72.

vitaly-krugl commented 9 years ago

Hi @awestendorf, I would like to start working with haigha ASAP, but I am blocked on the lack of basic.return support that this PR corrects. Would you mind taking a stab at merging this PR and cutting a new release? The change is small and pretty straightforward.

P.S., there is also a complimentary stability PR #74, but that one is less pressing.

awestendorf commented 9 years ago

Overall I like these changes and all the documentation and tests you included. I haven't confirmed that the implementation of recv_return matches the spec.

vitaly-krugl commented 9 years ago

I tested the return functionality manually against a RabbitMQ broker running on my Mac. I noticed that there are no acceptance/integration tests in haigha, and occasionally an error slips in despite the unit tests. I am guessing that it was a conscious decision not to have acceptance tests. What was the reasoning? Thx.

Oh, sorry -- is the stuff in the scripts directory the equivalent of acceptance tests?

vitaly-krugl commented 9 years ago

Hi Aaron, the PR is now consistent with your feedback. I wrote up comments explaining why I think the new API method is appropriate/consistent and necessary for the non-publisher-acknowledgments case and also documented an idea for extending the implementation to support closures in the pub-ack use case (puback_cb). Is anything else outstanding with this PR? Thx

awestendorf commented 9 years ago

This looks good.

vitaly-krugl commented 9 years ago

Great, thanks!