twingly / twingly-amqp

:bus: Ruby gem for RabbitMQ subscribing and publishing
0 stars 0 forks source link

Various warnings #85

Closed walro closed 3 years ago

walro commented 3 years ago

I ran with bundle exec rspec --warnings and that yields a few different warnings:

warning: /Users/robin/Workspace/twingly-amqp/lib/twingly/amqp/connection.rb:1: warning: loading in progress, circular require considered harmful - /Users/robin/Workspace/twingly-amqp/lib/twingly/amqp.rb

/Users/robin/Workspace/twingly-amqp/lib/twingly/amqp/subscription.rb:65: warning: instance variable @cancel not initialized

/Users/robin/.gem/ruby/2.7.1/gems/bunny-2.17.0/lib/bunny/channel.rb:1594: warning: instance variable @recover_cancelled_consumers not initialized
walro commented 3 years ago

The last one originates from Bunny (https://github.com/ruby-amqp/bunny/blob/2.17.0/lib/bunny/channel.rb#L1593-L1595), so might be hard to get to. It could be that we're misusing some feature though, so worth investigating still.

Chrizpy commented 3 years ago

To answer the question about circular require. I can see that in amqp.rb https://github.com/twingly/twingly-amqp/blob/7b8fa545f336a072bd5174b6bd4c86eafee81f1b/lib/twingly/amqp.rb#L3 that we do a require on connection.rb however, over at connection.rb https://github.com/twingly/twingly-amqp/blob/7b8fa545f336a072bd5174b6bd4c86eafee81f1b/lib/twingly/amqp/connection.rb#L1 We do a require back to amqp.rb, I tried commenting out the require over in connection.rb I thought seeing as they're both in the same module, it should be fine. The specs turned green and the warning is gone.

Chrizpy commented 3 years ago

The warning about @cancel not being initialized. https://github.com/twingly/twingly-amqp/blob/7b8fa545f336a072bd5174b6bd4c86eafee81f1b/lib/twingly/amqp/subscription.rb#L63-L71 This is the only place @cancel shows up, there is a chance of @cancel being returned when not initialized, if cancel? is called rather than cancel!. Maybe we should initialize it with false seing as cancel! sets it to true?

walro commented 3 years ago

We do a require back to amqp.rb, I tried commenting out the require over in connection.rb I thought seeing as they're both in the same module, it should be fine. The specs turned green and the warning is gone.

I think that could lead to the return of #59, worst case. Perhaps we need to re-think the way things are required in this gem. I believe we tried to make every class require just what it needs, so that users of the gem can require certain parts only. Not entirely sure what best practices is, perhaps one can look at other gems for inspiration.

This is the only place @cancel shows up, there is a chance of @cancel being returned when not initialized, if cancel? is called rather than cancel!. Maybe we should initialize it with false seing as cancel! sets it to true?

Sure, adding @cancel = false in the initializer makes sense to me.

walro commented 3 years ago

Bumping to critical as the circular reference warning is really intrusive when running tight TDD loops.

Pontus4 commented 3 years ago

Resolved in #87