Merge lp:~pedro.baeza/banking-addons/7.0-bank-statement-reconcile-prof_parsing into lp:banking-addons/bank-statement-reconcile-70

[Pedro Manuel Baeza]

Simple improvement on the account_statement_base_import module to have profile data on parse method to use it to customize parsing with, for example, a parameter introduced on profiles.

There is no need to do it anything more, because parse method already contains *args and **kwargs. You can access profile data with args[0] in parse method.

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

Next time someone review this module, he will wander why prof is passed to parser and why it is unused.

You should at least add a comment if you don't want it to be removed. Otherwise you should create a hook.

review: Needs Fixing

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Yannick,

I have added a comment to explain the purpose of prof variable.

Thanks for you review.

Regards.

[Laurent Mignon (Acsone) (lmi)]

Hi,

Why not spend the profile to the parser factory in place of to the parse method?

Regards,

lmi

[Pedro Manuel Baeza (pedro.baeza)]

I have made it in this way to make little changes, but that can be another solution. What do you prefer?

Regards.

[Laurent Mignon (Acsone) (lmi)]

I'm a little bit uneasy with the proposed merge since part of the profile information is spend to the parser factory (import_type) and the profile itself to the parse method. We can imagine use cases where information from the profile could be required to initialize the parser. I would prefer to spent the profile object at the earlier to the parser.
It's just my point of view ...

Regards

> I have made it in this way to make little changes, but that can be another
> solution. What do you prefer?
>
> Regards.

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

Hi Pedro,

Thanks for the contrib. After thinking about, I must admit I'm in favor of Laurent's suggestion here. Can you make the changes please ?

Regards,

review: Approve (code review, no tests)

[Pedro Manuel Baeza (pedro.baeza)]

I have changed the approach taking in consideration Yannick's comments. Please check it this new approach.

Regards.

[Laurent Mignon (Acsone) (lmi)]

Hi Pedro,

Thanks for the contrib and the changes!

LGTM,

Regards,

lmi

review: Approve (code review, no tests)

[Pedro Manuel Baeza (pedro.baeza)]

Sorry, it was Laurent's comments, not Yannick's comments.

Regards.

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

Doc string should be updated and signature seems wrong at l.47 parser_name is missing

Cheers.

review: Needs Fixing (code review, no test)

[lp:~pedro.baeza/banking-addons/7.0-bank-statement-reconcile-prof_parsing]

144. By Pedro Manuel Baeza on 2014-05-12

Docstring and change in parser factory params

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Yannick, see if this fits your requests.

Thanks for the review.

Regards.

[Yannick Vaucher @ Camptocamp (yvaucher-c2c)]

You must also ensure it remains compatible with child classes.

account_statement_ofx_import and account_statement_transactionid_import depends on account_statement_base_import

One inherit directly and the other indirecly from parser defined in account_statement_base_import.

[lp:~pedro.baeza/banking-addons/7.0-bank-statement-reconcile-prof_parsing]

145. By Pedro Manuel Baeza on 2014-05-12

Change account_statement_transactionid_import and account_statement_ofx_import accordingly.

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Yannick,

That was the reason why I didn't want signature at first instance, but it doesn't matter. I have changed both modules, even cleaning a bit ofx_import, because __init__ method is not needed.

Regards.

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

Hi Pedro,

Thanks for changing this !

LGTM,

Regards

review: Approve (code review, no tests)

[Pedro Manuel Baeza (pedro.baeza)]

Transfered to GitHub:

7.0 branch: https://github.com/OCA/bank-statement-reconcile/pull/4
master branch: https://github.com/OCA/bank-statement-reconcile/pull/3

Unmerged revisions

145. By Pedro Manuel Baeza on 2014-05-12

Change account_statement_transactionid_import and account_statement_ofx_import accordingly.

144. By Pedro Manuel Baeza on 2014-05-12

Docstring and change in parser factory params

143. By Pedro Manuel Baeza on 2014-05-09

[IMP] account_statement_base_import: Profile passed and stored on parser initialisation instead of an argument for parse method

142. By Pedro Manuel Baeza on 2014-05-05

[IMP] account_statement_base_import: Comment for prof variable.

141. By Pedro Manuel Baeza on 2014-04-05

[IMP] account_statement_base_import: Allow to use profile data for customizing parsing

=== modified file 'account_statement_base_import/parser/parser.py'
--- account_statement_base_import/parser/parser.py	2014-02-28 13:02:35 +0000
+++ account_statement_base_import/parser/parser.py	2014-05-12 13:41:04 +0000
@@ -42,14 +42,16 @@
     from the FileParser instead.
     """
 
-    def __init__(self, parser_name, *args, **kwargs):
+    def __init__(self, profile, *args, **kwargs):
         # The name of the parser as it will be called
-        self.parser_name = parser_name
+        self.parser_name = profile.import_type
         # The result as a list of row. One row per line of data in the file, but
         # not the commission one !
         self.result_row_list = None
         # The file buffer on which to work on
         self.filebuffer = None
+        # The profile record to access its parameters in any parser method
+        self.profile = profile
         self.balance_start = None
         self.balance_end = None
         self.statement_name = None
@@ -210,13 +212,13 @@
                 yield sub
 
 
-def new_bank_statement_parser(parser_name, *args, **kwargs):
-    """
-    Return an instance of the good parser class base on the providen name
-    :param char: parser_name
-    :return: class instance of parser_name providen.
+def new_bank_statement_parser(profile, *args, **kwargs):
+    """Return an instance of the good parser class based on the given profile.
+
+    :param profile: browse_record of import profile.
+    :return: class instance for given profile import type.
     """
     for cls in itersubclasses(BankStatementImportParser):
-        if cls.parser_for(parser_name):
-            return cls(parser_name, *args, **kwargs)
+        if cls.parser_for(profile.import_type):
+            return cls(profile, *args, **kwargs)
     raise ValueError

=== modified file 'account_statement_base_import/statement.py'
--- account_statement_base_import/statement.py	2014-05-07 08:50:38 +0000
+++ account_statement_base_import/statement.py	2014-05-12 13:41:04 +0000
@@ -156,7 +156,7 @@
                                  _("You must provide a valid profile to import a bank statement!"))
         prof = prof_obj.browse(cr, uid, profile_id, context=context)
 
-        parser = new_bank_statement_parser(prof.import_type, ftype=ftype)
+        parser = new_bank_statement_parser(prof, ftype=ftype)
         result_row_list = parser.parse(file_stream)
         # Check all key are present in account.bank.statement.line!!
         if not result_row_list:

=== modified file 'account_statement_ofx_import/parser/ofx_parser.py'
--- account_statement_ofx_import/parser/ofx_parser.py	2013-11-04 12:22:39 +0000
+++ account_statement_ofx_import/parser/ofx_parser.py	2014-05-12 13:41:04 +0000
@@ -29,14 +29,7 @@
     raise Exception(_('Please install python lib ofxparse'))
 
 class OfxParser(BankStatementImportParser):
-    """
-    Class for defining parser for OFX file format.
-    """
-    
-    def __init__(self, parser_name, *args, **kwargs):
-        """
-        """
-        super(OfxParser, self).__init__(parser_name, *args, **kwargs)
+    """Class for defining parser for OFX file format."""
 
     @classmethod
     def parser_for(cls, parser_name):

=== modified file 'account_statement_transactionid_import/parser/transactionid_file_parser.py'
--- account_statement_transactionid_import/parser/transactionid_file_parser.py	2013-11-06 15:23:49 +0000
+++ account_statement_transactionid_import/parser/transactionid_file_parser.py	2014-05-12 13:41:04 +0000
@@ -27,7 +27,7 @@
     bank statement.
     """
 
-    def __init__(self, parse_name, ftype='csv', extra_fields=None, header=None, **kwargs):
+    def __init__(self, profile, ftype='csv', extra_fields=None, header=None, **kwargs):
         """
         Add transaction_id in header keys
             :param char: parse_name: The name of the parser
@@ -37,7 +37,7 @@
             :param list: header : specify header fields if the csv file has no header
             """
         extra_fields = {'transaction_id': unicode}
-        super(TransactionIDFileParser, self).__init__(parse_name, extra_fields=extra_fields,
+        super(TransactionIDFileParser, self).__init__(profile, extra_fields=extra_fields,
                                                       ftype=ftype, header=header, **kwargs)
         # ref is replaced by transaction_id thus we delete it from check
         self.keys_to_validate = [k for k in self.keys_to_validate if k != 'ref']