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: boun...@canonical.com [mailto:boun...@canonical.com] On Behalf Of Guewen Baconnier @ Camptocamp Sent: donderdag 19 december 2013 8:40 To: mp+199...@code.launchpad.net 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. -- Mailing list: https://launchpad.net/~openerp-community-reviewer Post to : openerp-community-reviewer@lists.launchpad.net Unsubscribe : https://launchpad.net/~openerp-community-reviewer More help : https://help.launchpad.net/ListHelp