Code review comment for lp:~elachuni/txamqp/heartbeat

> 1 - The virtual host was changed to "/", instead of "localhost".

Yikes, that was unintentionally left in from running the tests :-/
I've put the 'localhost' vhost back in rev.22, sorry for that.

> 2 - Although the code works and is clear, I'd rather use a LoopingCall (http:/
> /twistedmatrix.com/documents/8.2.0/api/twisted.internet.task.LoopingCall.html)
> . It's already built into Twisted, so you don't need to implement things like
> lastSent, lastReceived, etc. I think it would be a bit easier to do something
> like this (it's just pseudo-python, can't guarantee that it works):
>
> (...code...)
>
> What do you think guys? The changes are minimal and I think they are a bit
> easier to read.

I gave your code a try and it works well, with a couple of changes. However I don't really see much benefit in using LoopingCall in this case:
 * checkHB could use a regular callLater call, as you've pointed out.
 * The rescheduling of sendHB should actually be done in sendFrame, not in processFrame, as it's sending
frames (not receiving them) that postpones the need to send a HB frame.
 * After changing that it's practically the same to be using a regular callLater instead of a LoopingCall for sendHB; if in sendFrame you reschedule sendHB whatever the payload type (instead of filtering out HBs) it effectively behaves like a LoopingCall.
 * We do get rid of lastSent and lastReceived, but instead we'd be using two callbacks instead of one (checkHeartbeat and sendHeartbeat vs. heartbeatHandler), and two 'callback handles' (checkHB and sendHB vs. pendingHeartbeat).
 * The free bonus you get with lastSent and lastReceived is a bit of accountability about how long ago you sent and received frames (including heartbeat frames), which makes it easier to test.

I haven't pushed these changes into this branch, but if you want I can push a working version using LoopingCalls on some other branch so we can compare.