openerp-community-reviewer team mailing list archive

["Guewen Baconnier @ Camptocamp" <guewen.baconnier@xxxxxxxxxxxxxx>]

Review: Needs Fixing

Hi,

Why do you wrote this condition like that:

    'accrued' not in context.get('type', '-') and True or False

Can't you write?

    'accrued' not in context.get('type')

l.510 and some other places:

  if len(ids) != 1: raise

You can't do that, 'raise' without exception is solely used to reraise the *current* exception in an exception handler. Here, it will fail with a TypeError. Prefer:

  assert len(ids) == 1

l.524-525,1814 you don't need to check if to_delete_line_ids is empty because unlink will return early if it was.

l.530: remove .keys():  if cur_cutoff['type'] not in pick_type_map:
l.531  raise # this should never happen => as before, raise alone can be used here, in fact you want to do:

    assert cur_cutoff['type'] in pick_type_map

l.509 get_lines_from_picking()
l.989 create_move()
l.1802 get_prepaid_lines()
Aren't these methods good candidates for '_prepare' methods?

l.1043 context['account_period_prefer_normal'] = True
The context can be None; it must be copied before you alter it to avoid propagation upstream, so you may want to add:
    if context is None:
        context = {}
    else:
        context = context.copy()

l.1656-1668, 1688-1702: I don't think that's allowed and valid to raise errors inside a _constraints, but I may be wrong. In any case, in orm.py there is no error handling around the _constraints functions so the exception will be caught by the general exception handler and we'll loose the handling of the _constraint ('Error occured while validating the fields...').


Overall: 
 - some lines too long
 - no space before colon (e.g. 'Error:', not 'Error :')
 - you don't need to use .keys() on the dictionaries when you work with the keys
 - On the indentation (pep8):

    to_delete_line_ids = line_obj.search(cr, uid, [
        ('parent_id', '=', cur_cutoff['id']),
        ('stock_move_id', '!=', False)
        ], context=context) or
    'account_id': fields.many2one('account.account', 'Regular Account',
        domain=[('type','<>','view')], required=True),

   are wrong ("Arguments on first line forbidden when not using vertical alignment"). Please refer to http://www.python.org/dev/peps/pep-0008/#indentation
 - on the blank lines (pep8): "Separate top-level function and class definitions with two blank lines.", "Method definitions inside a class are separated by a single blank line.", more at: http://www.python.org/dev/peps/pep-0008/#blank-lines

(I put the last 2 points to pep8 for the reference so you know, I won't refuse the merge because you have an extra blank line)

Thanks for your work!
-- 
https://code.launchpad.net/~akretion-team/account-closing/70-cutoff-modules/+merge/185992
Your team Account Core Editors is subscribed to branch lp:account-closing.