banking-addons-team team mailing list archive

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

Review: Needs Fixing code review, no test

"remove sapce at the end of lines due to my IDE"
Can you remove this option from your IDE please?

You removed the mutable in (l.11) the keyword argument, which is correct. But I don't think the change of the line 52 is fine. I would prefer to initialize `self.keys_to_validate` to an empty list when the argument is None.  Thus, the change is backward compatible and you don't need to verify the attribute everywhere it could be used.

l.78: the del on the kwargs could fail with a KeyError

l.318: fixing the typo errors in method names is fine, but I think you should ensure backward compatibility at least for public methods, keeping the old method which delegate the call to the new one (ideally with a warning).

Thanks for your MP
-- 
https://code.launchpad.net/~akretion-team/banking-addons/bank-statement-reconcile-61-ref-base-import/+merge/159104
Your team Banking Addons Team is subscribed to branch lp:banking-addons/bank-statement-reconcile-61.