openerp-community-reviewer team mailing list archive

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

Review: Approve code review

Hello,
thanks for your proposal.

Here is a list things that could be improved. I still approve because I don't think they should block the merge.


--

Some pep8 warnings.
--

l.867-899: backslash can be removed, indentation (just move SELECT down-left or vertical align all the lines below SELECT)
--

Usage of list could often be replaced by tuples, examples:

    if journal.type in ['sale', 'sale_refund', 'purchase', 'purchase_refund']:
could be
    if journal.type in ('sale', 'sale_refund', 'purchase', 'purchase_refund'):
and
    return [select_extra, join_extra, where_extra]
could be
    return (select_extra, join_extra, where_extra)
and so on.
--

map() on lines 906-920 would be more readable using for loops.
In other places, map() is often used when it could be replaced by list comprehensions.
--

l.962:
    for i, line in enumerate(lines_in):
is better than
    for i in range(1, line_cnt):
        line = lines_in[i]

I saw this idiom several times along the code.

--


Finally a question: why do you write "All rights reserved." in the header? Seems just wrong to me.
-- 
https://code.launchpad.net/~luc-demeyer/account-financial-report/7.0-account_journal_report_xls/+merge/199546
Your team Account Report Core Editors is subscribed to branch lp:account-financial-report.