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

BunnyMock::Queue#pop api does not match Bunny::Queue#pop #15

Open
podung opened this issue May 15, 2016 · 3 comments
Open

BunnyMock::Queue#pop api does not match Bunny::Queue#pop #15

podung opened this issue May 15, 2016 · 3 comments

Comments

@podung
Copy link
Contributor

podung commented May 15, 2016

I see that BunnyMock::Queue#pop returns a hash with :message and :options keys:

xchg.publish("Hello, everybody!", :routing_key => 'test1')

message = q.pop
message[:message]    # => 'Hello, everybody!'
message[:options]    # => { routing_key: 'test1' }

However Bunny::Queue#pop returns a 3 element array:

xchg.publish("Hello, everybody!", :routing_key => 'test1')

delivery_info, properties, payload = q.pop

I assume that this was to make popping messages off of queues in specs a lot cleaner. However, it makes BunnyMock only really able to help test the publishing of messages. Any code that attempts to use Bunny to read messages from queues would not be able to use BunnyMock.new as a replacement for Bunny.new.

Ideally the APIs would match, however changing it would break anyone using the current behavior. I propose creating some sort of config option (e.g. BunnyMock.use_bunny_queue_pop_api = true) that could default to the current behavior. The README could clearly show how to use the two options.

Thoughts? Let me know if I'm misunderstanding something.

I'll put together a PR if you're open to the idea.

@arempe93
Copy link
Owner

Yeah, this is leftover from when this was a personal use thing before I made it into a gem - because I didn't like the 3 element array style...probably not a good idea in hind sight lol. The apis should definitely match - so it can be used as a drop in replacement.

I think your idea is a good one, but we should add a deprecation notice to it - ie. if they keep using the current behavior they will know it will be phased out in a later version and be only available as the Bunny api behavior.

@arempe93
Copy link
Owner

@podung - see 1d2e57c

I took your idea and implemented it - along with adding a ton more stuff to match Bunny such as GetResponse and MessageProperties implementations and passing options and blocks to Queue#pop

@podung
Copy link
Contributor Author

podung commented May 18, 2016

Awesome. I haven't had a chance to look at this thoroughly yet, but thanks for being open to moving in this direction.

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

No branches or pull requests

2 participants