Merge lp:~therp-nl/banking-addons/ba70-lp1231174-default_method_should_not_raise_at_module_installation_time into lp:banking-addons

[Stefan Rijnhart (Opener)]

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Stefan, can you use the v7 form "from openerp.osv.orm import except_orm"? I also prefer the more compact "context = context or {}" instead of the two lines "if context is None...".

Regards.

review: Needs Fixing (code review, no test)

[Pedro Manuel Baeza (pedro.baeza)]

Another thing: if date is False, has any meaning to call "self.pool.get('account.period').find(cr, uid, dt=date, context=local_ctx)[0]"?

If not, I would refactor this to:

if not date:
    return False
else:
    self.pool.get('account.period').find...

And I suppose that the try is not neccessary in this case...

review: Needs Information (code review, no test)

[Stefan Rijnhart (Opener) (stefan-opener)]

Hi Pedro,

thank you for the review! I know that osv.except_osv is deprecated, but this is what account_period.find() raises, so that is what I have to catch.

I actually prefer the test for context is None, otherwise I am reconstructing an empty dictionary even if the passed context is already an empty dictionary.

If date is False, account_period.find() returns the period of the current date, so I can't refactor according to your suggestion.

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Stefan, thank you very much for your comments that teach me another thing. You are right about the recreation on the dictionary, but I'm not sure if the computation price is much higher than the test for None value, does it worth?

Regards.

review: Approve (code review, no test)

[Stefan Rijnhart (Opener) (stefan-opener)]

It's not just the computation. Some code higher up in the chain might depend on the mutatiblity of the context, execting to find its contents changed after some function call (which would constitute an ugly hack for sure, but these can be necessary at times)

[Pedro Manuel Baeza (pedro.baeza)]

Uf, indeed what an ugly hack! I haven't seen this yet, but if you say so...

Thanks for teaching me!

[Ronald Portier (Therp) (rportier1962)]

Amazingly the one line check for context is faster then the twoline check, even though recreating the empty dictionary. The differences are however very small. It also does not make a big difference wether a context is passed or not.

See the little script below. I have no clue about why this is.

For the functional reasons outlined by Stephan, the twoline check might still be preferred.

=======================
script time_context.py:
=======================

#!/usr/bin/env python
def one_line_context(context=None):
    context = context or {}

def two_line_context(context=None):
    if context == None:
        context = {}

if __name__ == '__main__':
    import timeit
    # Test calling context method without context
    tics = timeit.timeit(
        'one_line_context()',
        setup='from __main__ import one_line_context',
        number=10000)
    print 'Oneline withouth context takes %f tics' % (tics,)
    tics = timeit.timeit(
        'two_line_context()',
        setup='from __main__ import two_line_context',
        number=10000)
    print 'Twoline withouth context takes %f tics' % (tics,)

    # Test calling context method with context
    context = {'testing': True}
    tics = timeit.timeit(
        'one_line_context(context=context)',
        setup='from __main__ import one_line_context, context',
        number=10000)
    print '\nOneline with context takes %f tics' % (tics,)
    tics = timeit.timeit(
        'two_line_context(context=context)',
        setup='from __main__ import two_line_context, context',
        number=10000)
    print 'Twoline with context takes %f tics' % (tics,)

    # Test calling context method with empty context
    empty_context = {}
    tics = timeit.timeit(
        'one_line_context(context=empty_context)',
        setup='from __main__ import one_line_context, empty_context',
        number=10000)
    print '\nOneline with empty context takes %f tics' % (tics,)
    tics = timeit.timeit(
        'two_line_context(context=empty_context)',
        setup='from __main__ import two_line_context, empty_context',
        number=10000)
    print 'Twoline with empty context takes %f tics' % (tics,)

    print '\nNow decide on performance results'

======================
Results of running it:

Oneline withouth context takes 0.002138 tics
Twoline withouth context takes 0.007042 tics

Oneline with context takes 0.002146 tics
Twoline with context takes 0.002944 tics

Oneline with empty context takes 0.003464 tics
Twoline with empty context takes 0.004271 tics

Now decide on performance results

[Ronald Portier (Therp) (rportier1962)]

Just to continue on my last comment, even though it is a little bit off topic.

The test context == None in my test script should of course be replaced by context is None [thanks flake8], so is instead of ==.

Doing this makes the two line version of the code faster in case an empty context is passed. Otherwise the two line version is still slower.

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Ronald, thank you very much for your deep exploration on this topic. I see no clear winner then. I still preferred the one line option, and avoid hacks like ones mentioned by Stefan, but it's a matter of taste ;)

Regards.

[Stéphane Bidoul (Acsone) (sbi)]

Hi Pedro, all,

Irrespective of the performance aspects, it's more than a matter of taste
IMHO. If a method receives a dict in the context, even an empty one, the
same dict must be used, otherwise changes to the context would not be
propagated to the caller.

Communicating to the caller through the context may be hackish, but it's
the behaviour we expect when changing the context.

So context = context or {} would not be 100% correct.
Correct, albeit less readable, one liners would be
context = context if context is not None else {}
context = {} if context is None else context

My 2 cents.

-sbi

On Fri, Sep 27, 2013 at 1:59 AM, Pedro Manuel Baeza
<email address hidden>wrote:

> Hi, Ronald, thank you very much for your deep exploration on this topic. I
> see no clear winner then. I still preferred the one line option, and avoid
> hacks like ones mentioned by Stefan, but it's a matter of taste ;)
>
> Regards.
> --
>
> https://code.launchpad.net/~therp-nl/banking-addons/ba70-lp1231174-default_method_should_not_raise_at_module_installation_time/+merge/187677
> You are subscribed to branch lp:banking-addons.
>

[Stéphane Bidoul (Acsone) (sbi)]

LGTM

review: Approve (code review, no test)

[Pedro Manuel Baeza (pedro.baeza)]

Hi, Stéphane, I understand your reasoning. I'll keep the two lines sentence then.

Thanks, and sorry for the off-topic.

Regards.

[Holger Brunn (Therp) (hbrunn)]

review: Approve (code review)

=== modified file 'account_banking/account_banking.py'
--- account_banking/account_banking.py	2013-06-25 15:40:51 +0000
+++ account_banking/account_banking.py	2013-09-26 07:03:25 +0000
@@ -63,6 +63,7 @@
 '''
 
 from openerp.osv import orm, fields
+from openerp.osv.osv import except_osv
 from openerp.tools.translate import _
 from openerp import netsvc, SUPERUSER_ID
 from openerp.addons.decimal_precision import decimal_precision as dp
@@ -468,9 +469,27 @@
     _description = 'Bank Transaction'
 
     def _get_period(self, cr, uid, context=None):
-        date = context.get('date', None)
-        periods = self.pool.get('account.period').find(cr, uid, dt=date)
-        return periods and periods[0] or False
+        """
+        Get a non-opening period for today or a date specified in
+        the context.
+
+        Used in this model's _defaults, so it is always triggered
+        on installation or module upgrade. For that reason, we need
+        to be tolerant and allow for the situation in which no period
+        exists for the current date (i.e. when no date is specified).
+        """
+        if context is None:
+            context = {}
+        date = context.get('date', False)
+        local_ctx = dict(context)
+        local_ctx['account_period_prefer_normal'] = True
+        try:
+            return self.pool.get('account.period').find(
+                cr, uid, dt=date, context=local_ctx)[0]
+        except except_osv:
+            if date:
+                raise
+        return False
 
     def _get_currency(self, cr, uid, context=None):
         '''