Merge lp:~akretion-team/banking-addons/account_statement_base_import_conversion_dict into lp:banking-addons/bank-statement-reconcile-70

[Florian da Costa]

The conversion dict is displaced from the file_parser to the generic_parser in order to avoid the error of the validate function when creating a new parser which does not contain the 4 basc fields (ref, amount, date, label).

[lp:~akretion-team/banking-addons/account_statement_base_import_conversion_dict]

104. By Florian da Costa on 2013-12-09

[FIX] fix init function definition in order it works with conversion_dict

105. By Florian da Costa on 2013-12-09

Remove useless code

[Sébastien BEAU - http://www.akretion.com (sebastien.beau)]

I approve this merge.
Note the key "extra_fields" have been introduced here : http://bazaar.launchpad.net/~banking-addons-team/banking-addons/bank-statement-reconcile-70/revision/94#account_statement_base_import/parser/file_parser.py
I thinks it's better to come back with the convertion_dict because the FileParser is an abstract Parser for file, and can be reuse in many case (Paypal, Atos, Paybox...). So the convert dict depend of the implementation of the Parser and can not be generic

review: Approve (code review, no test)

[Sébastien BEAU - http://www.akretion.com (sebastien.beau)]

Hum, I read to fast.
The import of "float_or_zero" is missing in the generic_parser.

Add
"""
from openerp.addons.account_statement_base_import.parser.file_parser.py import float_or_zero
"""

Also test the generic import in order to be sure that everything is ok

review: Needs Fixing

[lp:~akretion-team/banking-addons/account_statement_base_import_conversion_dict]

106. By Florian da Costa on 2013-12-09

[FIX] Import float_or_zero function in generic_parser (goes with the 2 previous commits)

107. By Florian da Costa on 2013-12-09

[FIX] Fix previous import

[Sébastien BEAU - http://www.akretion.com (sebastien.beau)]

It's ok for me now

review: Approve (code review, no test)

[Florian da Costa (florian-dacosta)]

I have tested the generic parser with a basic CSV file and it seems to work properly.

[Joël Grand-Guillaume @ camptocamp (jgrandguillaume-c2c)]

Hi,

Thanks for the contribs !

 * Line 57 miss spaces after comma (PEP8)

Otherwise, I would like to have Virgil Dupras and Guewen opinions on that one, I'm a bit affraid that we might break some code by changing the keyword argument from extra_field to conversion_dict.

For that main reason I set as need information.

review: Needs Fixing (code review, no tests)

[lp:~akretion-team/banking-addons/account_statement_base_import_conversion_dict]

108. By Florian da Costa on 2013-12-12

[FIX] miss space after comma (PEP8)

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

Why renaming the keyword argument extra_field in a not backward compatible manner?
I think it is unnecessary.

review: Needs Fixing

[lp:~akretion-team/banking-addons/account_statement_base_import_conversion_dict]

109. By Florian da Costa on 2014-01-31

[FIX] Replace conversion_dict by extra_fields

[Florian da Costa (florian-dacosta)]

Well, the self.conversion_dict is initialized in the file_parser by the argument coming from the generic parser, that's why I don't understand why we are talking of "extra" fields.

And the name's changement comes from a recent commit :
http://bazaar.launchpad.net/~banking-addons-team/banking-addons/bank-statement-reconcile-70/revision/94#account_statement_base_import/parser/file_parser.py

I thought it was a mistake.

I re-changed the keword argument for extra_field, is it ok now?

[Florian da Costa (florian-dacosta)]

Is it possible to have news for this MP?

I guess the name of the keyword is not important to me, it can be extra_field.
The important thing is to move the conversion_dict containing the file's fields from file_parser to generic parser.
Because, the goal of the file_parser is not to parse a particular file mais should be used for files very differents which could eventually not contain 'ref', 'label', 'amount'... right?

So the fields'name of the file should always be in the custom parser (or generic parser).

Or maybe I mis-understood the goal of these parsers.
Thanks

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Florian, I see your changes correct now, so I give my appoval.

Thanks.

Regards.

review: Approve (code review)

[Florian da Costa (florian-dacosta)]

Guewen and Joël, does it seem ok for you now?

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

LGTM

review: Approve

[Pedro Manuel Baeza (pedro.baeza)]

This project is now hosted on https://github.com/OCA/bank-statement-reconcile. Please move your proposal there. This guide may help you https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub

Unmerged revisions

109. By Florian da Costa on 2014-01-31

[FIX] Replace conversion_dict by extra_fields

108. By Florian da Costa on 2013-12-12

[FIX] miss space after comma (PEP8)

107. By Florian da Costa on 2013-12-09

[FIX] Fix previous import

106. By Florian da Costa on 2013-12-09

[FIX] Import float_or_zero function in generic_parser (goes with the 2 previous commits)

105. By Florian da Costa on 2013-12-09

Remove useless code

104. By Florian da Costa on 2013-12-09

[FIX] fix init function definition in order it works with conversion_dict

103. By Florian da Costa on 2013-12-09

[FIX] conversion_dict is now created in the generic parser in order to allow other parsers not to use file with ref, label, date, and amount keys.

=== modified file 'account_statement_base_import/parser/file_parser.py'
--- account_statement_base_import/parser/file_parser.py	2013-11-06 17:27:53 +0000
+++ account_statement_base_import/parser/file_parser.py	2014-01-31 12:14:31 +0000
@@ -42,7 +42,7 @@
         """
             :param char: parse_name: The name of the parser
             :param char: ftype: extension of the file (could be csv or xls)
-            :param dict: extra_fields: extra fields to add to the conversion dict. In the format
+            :param dict: extra_fields: extra fields to put into the conversion dict. In the format
                                      {fieldname: fieldtype}
             :param list: header : specify header fields if the csv file has no header
             """
@@ -53,14 +53,8 @@
         else:
             raise except_osv(_('User Error'),
                              _('Invalid file type %s. Please use csv or xls') % ftype)
-        self.conversion_dict = {
-            'ref': unicode,
-            'label': unicode,
-            'date': datetime.datetime,
-            'amount': float_or_zero,
-        }
-        if extra_fields:
-            self.conversion_dict.update(extra_fields)
+        self.conversion_dict = extra_fields
+
         self.keys_to_validate = self.conversion_dict.keys()
         self.fieldnames = header
         self._datemode = 0  # used only for xls documents,

=== modified file 'account_statement_base_import/parser/generic_file_parser.py'
--- account_statement_base_import/parser/generic_file_parser.py	2013-05-03 19:57:26 +0000
+++ account_statement_base_import/parser/generic_file_parser.py	2014-01-31 12:14:31 +0000
@@ -24,6 +24,7 @@
 import tempfile
 import datetime
 from file_parser import FileParser
+from openerp.addons.account_statement_base_import.parser.file_parser import float_or_zero
 try:
     import xlrd
 except:
@@ -38,7 +39,13 @@
     """
 
     def __init__(self, parse_name, ftype='csv', **kwargs):
-        super(GenericFileParser, self).__init__(parse_name, ftype=ftype, **kwargs)
+        conversion_dict = {
+            'ref': unicode,
+            'label': unicode,
+            'date': datetime.datetime,
+            'amount': float_or_zero,
+        }
+        super(GenericFileParser, self).__init__(parse_name, ftype=ftype, extra_fields=conversion_dict, **kwargs)
 
     @classmethod
     def parser_for(cls, parser_name):