Banking Addons

Merge lp://staging/~akretion-team/banking-addons/bank-statement-reconcile-61-ref-base-import into lp://staging/banking-addons/bank-statement-reconcile-61

  1. bank-statement-reconcile-61-ref-base-import
  2. Merge into bank-statement-reconcile-61
Proposed by Benoit Guillot - http://www.akretion.com
Status: Merged
Merged at revision: 81
Proposed branch: lp://staging/~akretion-team/banking-addons/bank-statement-reconcile-61-ref-base-import
Merge into: lp://staging/banking-addons/bank-statement-reconcile-61
Diff against target: 507 lines (+94/-74)
5 files modified
account_statement_base_import/parser/file_parser.py (+9/-8)
account_statement_base_import/parser/parser.py (+30/-26)
account_statement_base_import/statement.py (+47/-31)
account_statement_base_import/statement_view.xml (+1/-2)
account_statement_base_import/wizard/import_statement.py (+7/-7)
To merge this branch: bzr merge lp://staging/~akretion-team/banking-addons/bank-statement-reconcile-61-ref-base-import
Reviewer Review Type Date Requested Status
Guewen Baconnier @ Camptocamp code review, no test Approve
Holger Brunn (Therp) code review, no test Approve
Review via email: mp+159104@code.staging.launchpad.net

Description of the change

In this merge proposal, I submit some changes in the module account_statement_base_import.

 - Fix argument keys_to_validate in the init of the file_parser
 - add the possibility to force the dialect for the csv dictreader
 - fix the field : import_type of the class account_statement_profile, add a hook to add a new type
 - remove sapce at the end of lines due to my IDE
 - fix typo in the name of the method :prepare_statement_lines_vals

To post a comment you must log in.
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

"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

review: Needs Fixing (code review, no test)
Revision history for this message
Holger Brunn (Therp) (hbrunn) wrote :

as of line 406: you can indeed remove the unlink, as the transaction is rolled back anyway

lp://staging/~akretion-team/banking-addons/bank-statement-reconcile-61-ref-base-import updated
79. By Benoit Guillot - http://www.akretion.com

[IMP] fix keys_to_validate, remove useless unlink, fix dialect

Revision history for this message
Benoit Guillot - http://www.akretion.com (benoit-guillot-z) wrote :

I change with the several comments

Revision history for this message
Holger Brunn (Therp) (hbrunn) :
review: Approve (code review, no test)
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

LGTM

review: Approve (code review, no test)
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

Thanks Benoit.

I merged your proposal.

Did you created the MP for the version 7 somewhere?

Revision history for this message
Benoit Guillot - http://www.akretion.com (benoit-guillot-z) wrote :

Thanks Guewen for the merge !

I haven't created the MP for the v7.0 yet. Sebastien wants to make another MP on the v6.1, after that I will make the MP for the v7.0.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches