search

inway / mojo-rabbitmq-client

Mojo::IOLoop based RabbitMQ (AMQP) client
Artistic License 2.0
15 stars 17 forks source link

WIP: make Mojo::RabbitMQ::Client::Consumer use promises #23

Closed christopherraa closed 6 years ago

christopherraa commented 6 years ago

With this change Mojo::RabbitMQ::Client::Consumer uses promises. Please note that this is a work in progress and this PR represent a request for comment. This change could possibly, if accepted, close inway/mojo-rabbitmq-client#12 and inway/mojo-rabbitmq-client#22 .

christopherraa commented 6 years ago

Got some feedback from sri in the mojo irc-channel:

18:31 <@sri> mishanti1: any method that calls ->wait is wrong 18:31 <@sri> the user always decides when to use ->wait

Offending code is Mojo::RabbitMQ::Client::Consumer->start() that is implement just so the consumer looks like the publisher, ref Mojo::RabbitMQ::Client::Publisher->publish(). Perhaps it would be best to just remove start() in the consumer as there really is no blocking implementation there? Keep start_p() so that it is clear exactly what the implementation does?

What do you think @SPodjasek ?

SPodjasek commented 6 years ago

You're right consumer is by design long-running-task so there'll be no blocking behavior. One should use client methods get & get_p to synchronously get messages from queue.

christopherraa commented 6 years ago

@SPodjasek So am I interpreting you correctly if I suggest that ->start() should be removed in it's entirety?

christopherraa commented 6 years ago

@SPodjasek I have updated the fork with the changes in naming discussed yesterday. There are no tests for the functionality yet. If you want tests made I would greatly appreciate some guidelines from you on how you want the tests structured, plus to what extent you want tests.

christopherraa commented 6 years ago

@SPodjasek Any chance of getting this merged and moving closer to a new release? Are you waiting for anything from me?

tyldum commented 6 years ago

@SPodjasek Updates?