openerp-community-reviewer team mailing list archive

[Nicolas Bessi - Camptocamp <nicolas.bessi@xxxxxxxxxxxxxx>]

Review: Needs Fixing

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


-- 
https://code.launchpad.net/~camptocamp/carriers-deliveries/7.0-threaded-dispatch-label-generation/+merge/215184
Your team Stock and Logistic Core Editors is requested to review the proposed merge of lp:~camptocamp/carriers-deliveries/7.0-threaded-dispatch-label-generation into lp:carriers-deliveries.