Code review comment for lp:~camptocamp/carriers-deliveries/7.0-threaded-dispatch-label-generation

Revision history for this message
Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount) wrote :

Hello,

Thank for the patch

Small PEP8 in manifest also noticed that
the title underline is not correct

[Link module] Carrier labels - Picking dispatch
==============================

It should go to the end of previous line

sys is imported but not used

remark
q_except can be a simpler data cohntainer like a list has they are thread safe.

the nested try catch structure here is strange

        try:
            try:
                picking_out_obj.generate_labels(
                    thread_cr, uid, [picking.id],
                    tracking_ids=tracking_ids,
                    context=context)
            except orm.except_orm as e:
                # add information on picking and pack in the exception
                picking_name = _('Picking: %s') % picking.name
                pack_num = _('Pack: %s') % pack.name if pack else ''
                raise orm.except_orm(
                    e.name,
                    _('%s %s - %s') % (picking_name, pack_num, e.value))
            thread_cr.commit()
        except Exception:
            thread_cr.rollback()
            raise
        finally:
            thread_cr.close()

You should have only on try and many catches. and have your commit inside instruction block

Thanks

Nicolas

review: Needs Fixing

« Back to merge proposal