Review: Needs Fixing
Hi Erwin,
thank you for this contribution. Here are my remarks:
l.9 Shared copyright between you and Credativ seem appropriate here.
l.40 As I have only recently learned myself, you can take GPL code and apply
AGPL license to it. We will do so for 7.0. You might as well do it here now
(note the licence mentioned in l.13).
l.38, 49 Please provide a name and description in English and a translation in
the po file.
l.103 Please add a copyright notice.
l.105 This import is unused except in l.244 in raise_error which is unused in
itself. Please remove the import as well as the raise_error method (I am aware
the same error exists in HSBC).
l.162 Another copy/paste issue. Looks like you do have 'remote_account'
(l.129), although maybe not for each transfer type.
l.160 Invalid transactions are only logged to the server log, invisible to the
user who is importing a bank statement. Please set self.error_message in
is_valid itself.
l.249,250 Please mention Rabobank here in the name and code, as you do not seem
to aim at a generic 'MT940' parser here.
l.268 Dubious indentation. Align with the first 'r' in records in the previous
line instead
l.266 Dubious indentation. Align with the 'p' in parser in the previous line
instead.
l.278 Dummy list creation. How about something like:
for r in filter(None, records):
stmnt.import_record(r)
l.294 Looks like it's rather based on the HSBC parser than Fi Patu parser. In
fact, the changes are so minimal (and half of these are just style changes)
that you could consider a shared base and inherit that. At least add a
copyright notice. I suggest shared copyrights between Credativ and yourself (if
not the author of fi_patu).
Cheers,
Stefan.
--
https://code.launchpad.net/~endiansolutions/banking-addons/ab61-nl_rabo/+merge/141149
Your team Banking Addons Team is subscribed to branch lp:banking-addons.
--
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
