banking-addons-team team mailing list archive

Thread
Date

Re: lp:~akretion-team/banking-addons/bank-statement-reconcile-70-multi-statement into lp:banking-addons/bank-statement-reconcile-7.0

To: Sébastien BEAU - http://www.akretion.com <sebastien.beau@xxxxxxxxxxxx>
From: Joël Grand-Guillaume @ camptocamp <joel.grandguillaume@xxxxxxxxxxxxxx>
Date: Wed, 11 Dec 2013 15:03:26 -0000
In-reply-to: <20131204173118.32516.59655.launchpad@ackee.canonical.com>
Reply-to: mp+197761@xxxxxxxxxxxxxxxxxx
Sender: bounces@xxxxxxxxxxxxx

Review: Needs Fixing code review, no tests

Hi,

Thanks for the contribs !

My opinion on that one is that I prefer letting this method "def statement_import" as it is. His purpose is to import one single statement.

Why not simply adding a new method called "def multi_statement_import" that call the first one for each instance ?

This way, you reduce the risk of breaking others work by returning a dict instead of a int/long.

For this reason, I mark it as need fixing.

Regards,

Joël




-- 
https://code.launchpad.net/~akretion-team/banking-addons/bank-statement-reconcile-70-multi-statement/+merge/197761
Your team Banking Addons Core Editors is subscribed to branch lp:banking-addons/bank-statement-reconcile-7.0.