openerp-community-reviewer team mailing list archive

["Luc De Meyer \(Noviat\)" <luc.demeyer@xxxxxxxxx>]

Guewen,

Thanks for your fast review. 

I am trying to be the best student of the class this time and hence have uploaded a technical update of the code with
- pep8 warnings fixed
- backslash removals
- lists replaced by tuples where it makes sense
- map()replaced by list comprehension where it makes sense
- for i in range(1, line_cnt) replaced by  for i, line in enumerate(lines_in)

Regards,
Luc

P.S.

Concerning the 'All rights reserved." in the header:

We copied this from an OpenERP header  back in the old OpenERP V5 days. 
I am not a lawyer, but I don't think it hurts since the next paragraphs states that the software is under the AGPL license. 
If it's wrong, than we can of course remove these wordings.

-----Original Message-----
From: bounces@xxxxxxxxxxxxx [mailto:bounces@xxxxxxxxxxxxx] On Behalf Of Guewen Baconnier @ Camptocamp
Sent: donderdag 19 december 2013 8:40
To: mp+199546@xxxxxxxxxxxxxxxxxx
Subject: Re: [Merge] lp:~luc-demeyer/account-financial-report/7.0-account_journal_report_xls into lp:account-financial-report

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
You are the owner of lp:~luc-demeyer/account-financial-report/7.0-account_journal_report_xls.


-- 
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.