Code review comment for lp:~lucio.torre/graphite/graphite-add-rabbitmq

Revision history for this message
chrismd (chrismd) wrote :

I just looked through the code and it looks good, only a couple caveats. First is that the rest of the graphite codebase uses 2 space indentation whereas amqp_listener.py uses 4. I know that's the PEP-8 recommendation, I just prefer 2 spaces myself. I don't care too much if you want to leave it at 4.

Also it uses the @inlineCallbacks decorator which, while incredibly useful, is not compatible with python 2.4. I've been considering upping the python requirement to 2.5 simply because 2.4 is so old and I often find myself breaking compatibility on accident by using more recent features out of habit. So I don't worry about changing that yet, I'll probably make the next release require 2.5.

As for the xml file, is there not a free version of the AMQP spec? What do other txamqp apps use? I just checked out rabbitmq and it appears they have a JSON file describing the spec with a permissive license so we could use it. The only question is, can txamqp read it?

« Back to merge proposal