Patchwork [3,of,7,V2] transaction: add onclose/onabort hook for pre-close logic

login
register
mail settings
Submitter Durham Goode
Date March 31, 2014, 11:19 p.m.
Message ID <f85f9ea96d16e180de3e.1396307985@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/4168/
State Superseded
Commit 3c47677a8d04bc3bdc271a9ec36bbfc4f66781ed
Headers show

Comments

Durham Goode - March 31, 2014, 11:19 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1395701867 25200
#      Mon Mar 24 15:57:47 2014 -0700
# Node ID f85f9ea96d16e180de3e38cfc468e53441f41743
# Parent  a4fddbaf22f873d6d700be4b7d3842457d3680aa
transaction: add onclose/onabort hook for pre-close logic

Adds an optional onclose parameter to transactions that gets called just before
the transaction is committed. This allows things that build up data over the
course of the transaction (like the fncache) to commit their data.

Also adds onabort. It's not used, but will allow extensions to hook into onclose
and onabort to provide transaction support.
Pierre-Yves David - April 1, 2014, 12:07 a.m.
On 03/31/2014 04:19 PM, Durham Goode wrote:
> +        if self.count == 1 and self.onclose:
> +            self.onclose()

`onclose is not None`

Please, otherwise this is going to bit someone in 31.4 months

> @@ -149,6 +155,9 @@
>           self.usages = 0
>           self.file.close()
>
> +        if self.onabort:
> +            self.onabort()

`onabort is not None`

Please, otherwise this is going to bit someone in 28.3 months
Gregory Szorc - April 1, 2014, 12:14 a.m.
On 3/31/14, 5:07 PM, Pierre-Yves David wrote:
>
>
> On 03/31/2014 04:19 PM, Durham Goode wrote:
>> +        if self.count == 1 and self.onclose:
>> +            self.onclose()
>
> `onclose is not None`
>
> Please, otherwise this is going to bit someone in 31.4 months
>
>> @@ -149,6 +155,9 @@
>>           self.usages = 0
>>           self.file.close()
>>
>> +        if self.onabort:
>> +            self.onabort()
>
> `onabort is not None`

This is an anti-pattern in Python. Unless you are trying to 
differentiate None from False from [] from {} from '' from any other 
type whose __nonzero__ can return False, the explicit type comparison 
can be safely left out.
Pierre-Yves David - April 1, 2014, 12:16 a.m.
On 03/31/2014 05:14 PM, Gregory Szorc wrote:
> On 3/31/14, 5:07 PM, Pierre-Yves David wrote:
>>
>>
>> On 03/31/2014 04:19 PM, Durham Goode wrote:
>>> +        if self.count == 1 and self.onclose:
>>> +            self.onclose()
>>
>> `onclose is not None`
>>
>> Please, otherwise this is going to bit someone in 31.4 months
>>
>>> @@ -149,6 +155,9 @@
>>>           self.usages = 0
>>>           self.file.close()
>>>
>>> +        if self.onabort:
>>> +            self.onabort()
>>
>> `onabort is not None`
>
> This is an anti-pattern in Python. Unless you are trying to
> differentiate None from False from [] from {} from '' from any other
> type whose __nonzero__ can return False, the explicit type comparison
> can be safely left out.


For me, the contrary is the anti pattern. it is very easy to slip 
variable type to something that may exist and still be false.

I've lost multiple days in the last 5 years because of people not using 
`is not None`

(explicit is better than implicit)
Gregory Szorc - April 1, 2014, 12:24 a.m.
On 3/31/14, 5:16 PM, Pierre-Yves David wrote:
>
>
> On 03/31/2014 05:14 PM, Gregory Szorc wrote:
>> On 3/31/14, 5:07 PM, Pierre-Yves David wrote:
>>>
>>>
>>> On 03/31/2014 04:19 PM, Durham Goode wrote:
>>>> +        if self.count == 1 and self.onclose:
>>>> +            self.onclose()
>>>
>>> `onclose is not None`
>>>
>>> Please, otherwise this is going to bit someone in 31.4 months
>>>
>>>> @@ -149,6 +155,9 @@
>>>>           self.usages = 0
>>>>           self.file.close()
>>>>
>>>> +        if self.onabort:
>>>> +            self.onabort()
>>>
>>> `onabort is not None`
>>
>> This is an anti-pattern in Python. Unless you are trying to
>> differentiate None from False from [] from {} from '' from any other
>> type whose __nonzero__ can return False, the explicit type comparison
>> can be safely left out.
>
>
> For me, the contrary is the anti pattern. it is very easy to slip
> variable type to something that may exist and still be false.
>
> I've lost multiple days in the last 5 years because of people not using
> `is not None`
>
> (explicit is better than implicit)

I don't see how that applies here as you are calling self.onclose and 
self.onabort immediately after the check. That will raise if the thing 
isn't callable.
Pierre-Yves David - April 1, 2014, 12:25 a.m.
On 03/31/2014 04:19 PM, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1395701867 25200
> #      Mon Mar 24 15:57:47 2014 -0700
> # Node ID f85f9ea96d16e180de3e38cfc468e53441f41743
> # Parent  a4fddbaf22f873d6d700be4b7d3842457d3680aa
> transaction: add onclose/onabort hook for pre-close logic
>
> Adds an optional onclose parameter to transactions that gets called just before
> the transaction is committed. This allows things that build up data over the
> course of the transaction (like the fncache) to commit their data.
>
> Also adds onabort. It's not used, but will allow extensions to hook into onclose
> and onabort to provide transaction support.
>
> diff --git a/mercurial/transaction.py b/mercurial/transaction.py
> --- a/mercurial/transaction.py
> +++ b/mercurial/transaction.py
> @@ -42,12 +42,15 @@
>       opener.unlink(journal)
>
>   class transaction(object):
> -    def __init__(self, report, opener, journal, after=None, createmode=None):
> +    def __init__(self, report, opener, journal, after=None, createmode=None,
> +            onclose=None, onabort=None):
>           self.count = 1
>           self.usages = 1
>           self.report = report
>           self.opener = opener
>           self.after = after
> +        self.onclose = onclose
> +        self.onabort = onabort

Also, consider creating a docstring to this function and document the 
semantic and `onclose` and `onabort` in these
Pierre-Yves David - April 1, 2014, 12:26 a.m.
On 03/31/2014 05:24 PM, Gregory Szorc wrote:
> On 3/31/14, 5:16 PM, Pierre-Yves David wrote:
>>
>>
>> On 03/31/2014 05:14 PM, Gregory Szorc wrote:
>>> On 3/31/14, 5:07 PM, Pierre-Yves David wrote:
>>>>
>>>>
>>>> On 03/31/2014 04:19 PM, Durham Goode wrote:
>>>>> +        if self.count == 1 and self.onclose:
>>>>> +            self.onclose()
>>>>
>>>> `onclose is not None`
>>>>
>>>> Please, otherwise this is going to bit someone in 31.4 months
>>>>
>>>>> @@ -149,6 +155,9 @@
>>>>>           self.usages = 0
>>>>>           self.file.close()
>>>>>
>>>>> +        if self.onabort:
>>>>> +            self.onabort()
>>>>
>>>> `onabort is not None`
>>>
>>> This is an anti-pattern in Python. Unless you are trying to
>>> differentiate None from False from [] from {} from '' from any other
>>> type whose __nonzero__ can return False, the explicit type comparison
>>> can be safely left out.
>>
>>
>> For me, the contrary is the anti pattern. it is very easy to slip
>> variable type to something that may exist and still be false.
>>
>> I've lost multiple days in the last 5 years because of people not using
>> `is not None`
>>
>> (explicit is better than implicit)
>
> I don't see how that applies here as you are calling self.onclose and
> self.onabort immediately after the check. That will raise if the thing
> isn't callable.

until the code drift appart or some sort of strange evaluated to false 
but still callable object is added.

(special case are not special enough)

Patch

diff --git a/mercurial/transaction.py b/mercurial/transaction.py
--- a/mercurial/transaction.py
+++ b/mercurial/transaction.py
@@ -42,12 +42,15 @@ 
     opener.unlink(journal)
 
 class transaction(object):
-    def __init__(self, report, opener, journal, after=None, createmode=None):
+    def __init__(self, report, opener, journal, after=None, createmode=None,
+            onclose=None, onabort=None):
         self.count = 1
         self.usages = 1
         self.report = report
         self.opener = opener
         self.after = after
+        self.onclose = onclose
+        self.onabort = onabort
         self.entries = []
         self.map = {}
         self.journal = journal
@@ -126,6 +129,9 @@ 
     @active
     def close(self):
         '''commit the transaction'''
+        if self.count == 1 and self.onclose:
+            self.onclose()
+
         self.count -= 1
         if self.count != 0:
             return
@@ -149,6 +155,9 @@ 
         self.usages = 0
         self.file.close()
 
+        if self.onabort:
+            self.onabort()
+
         try:
             if not self.entries:
                 if self.journal: