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.

-- 
Mailing list: https://launchpad.net/~banking-addons-team
Post to     : [email protected]
Unsubscribe : https://launchpad.net/~banking-addons-team
More help   : https://help.launchpad.net/ListHelp

Reply via email to