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

Gregory Szorc gregory.szorc at gmail.com
Mon Mar 31 19:24:37 CDT 2014


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.


More information about the Mercurial-devel mailing list