Code review comment for lp:~camptocamp/report-print-send/6.1-pingen

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

Nice addition! Looks good on the whole. Just a couple of remarks:

Would it not be better to encode the dependency on 'requests' in the manifest file as

    'external_dependencies' : {
        'python' : ['requests'],
    }

That would give a friendly error message upon installation in OpenERP if this module is not available.

l.988,991: Are the integer dictionary keys stringified on purpose? Judging by the docstring of push_document you also accept integers there, so I would find it more intuitive to use native integers here. May be a matter of style though.

review: Needs Fixing

« Back to merge proposal