Code review comment for lp:~free.ekanayaka/landscape-client/amp-cleanup-6

Revision history for this message
Christopher Armstrong (radix) wrote :

Have you filed a bug about Twisted's AMP not supporting synchronous transports? I'm not really sure what that means, that's why I ask, so I can go check out the explanation on that bug.

[1] The code around the call to self._factory.connection.flush() is unclear to me. It *seems* that this code is only meant to be run in unit tests, not in normal execution, but the conditional (if the factory has a non-None connection) does not scream "testing-only" to me. Or is there a "flush" method on the real connection object that I'm unfamiliar with?

[2] It doesn't seem right that _socket_paths is a class attribute instead of an instance attribute

Everything else looks fine!

review: Approve

« Back to merge proposal