| 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
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
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.
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)
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.
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
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: