banking-addons-team team mailing list archive

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

Review: Needs Fixing code review

A global remark: OpenERP uses the word 'Journal Entries' for the moves, I think you should use the same word (for the UI strings).

A few typos:

in __openerp__.py

  s/allow to groupe/allows to group/

in __init__.py

  s/import statement/from . import statement/

in statement.py
  
  s/AccountStatementProfil/AccountStatementProfile/

Indentation + proposition of string/help message:  
    _columns = {
        'one_move': fields.boolean(
            'Group Journal Items', 
            help="Only one Journal Entry will be generated on the "
                 "validation of the bank statement.")
    }

l.190 if st.journal_id.default_debit_account_id is False, you will get st_line.account_id, is it the behavior you want ? (generally, using ...and...or... is considered a bad thing, but seems you used it intentionally to fallback on the st_line.account_id)

l.195ff just a little thing but while you are at it: the indentation should be vertically aligned

l.228 Will crash if a statement has 0 lines. It deserves an "assert st.line_ids" if this situation can't happen (in usual condition), or a check if the situation may happen (in usual condition)

l.229 Use != instead of <>

A few pep8 warnings when using a tool like flake8/pep8.

l.260: priority useless (default value is 16)
l.261: deprecated 'type' in view

Thanks!
-- 
https://code.launchpad.net/~akretion-team/banking-addons/bank-statement-reconcile-7.0-one-move/+merge/197769
Your team Banking Addons Core Editors is subscribed to branch lp:banking-addons/bank-statement-reconcile-7.0.