Patchwork registrar: raise a programming error on duplicated registering

login
register
mail settings
Submitter Pierre-Yves David
Date Dec. 16, 2016, 7:31 p.m.
Message ID <f3479f74af0d2b21c89e.1481916689@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/17937/
State Accepted
Headers show

Comments

Pierre-Yves David - Dec. 16, 2016, 7:31 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1481545965 -3600
#      Mon Dec 12 13:32:45 2016 +0100
# Node ID f3479f74af0d2b21c89e906c503f4cddf4b3c39b
# Parent  182cacaa4c32330c0466b7111a75d060830783e8
registrar: raise a programming error on duplicated registering

Previous, registering different object with the same name would silently
overwrite the first value with the second one. We now detect the situation and
raise an error. No extension in test or core had the issues.
Augie Fackler - Dec. 16, 2016, 7:45 p.m.
On Fri, Dec 16, 2016 at 08:31:29PM +0100, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1481545965 -3600
> #      Mon Dec 12 13:32:45 2016 +0100
> # Node ID f3479f74af0d2b21c89e906c503f4cddf4b3c39b
> # Parent  182cacaa4c32330c0466b7111a75d060830783e8
> registrar: raise a programming error on duplicated registering

Queued, thanks

>
> Previous, registering different object with the same name would silently
> overwrite the first value with the second one. We now detect the situation and
> raise an error. No extension in test or core had the issues.
>
> diff --git a/mercurial/registrar.py b/mercurial/registrar.py
> --- a/mercurial/registrar.py
> +++ b/mercurial/registrar.py
> @@ -8,6 +8,7 @@
>  from __future__ import absolute_import
>
>  from . import (
> +    error,
>      pycompat,
>      util,
>  )
> @@ -55,6 +56,9 @@ class _funcregistrarbase(object):
>              func._origdoc = doc
>              func.__doc__ = self._formatdoc(decl, doc)
>
> +        if name in self._table:
> +            raise error.ProgrammingError('duplicate registration for name %s',
> +                                         name)
>          self._table[name] = func
>          self._extrasetup(name, func, *args, **kwargs)
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Dec. 18, 2016, 7:24 a.m.
On Fri, 16 Dec 2016 20:31:29 +0100, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1481545965 -3600
> #      Mon Dec 12 13:32:45 2016 +0100
> # Node ID f3479f74af0d2b21c89e906c503f4cddf4b3c39b
> # Parent  182cacaa4c32330c0466b7111a75d060830783e8
> registrar: raise a programming error on duplicated registering
> 
> Previous, registering different object with the same name would silently
> overwrite the first value with the second one. We now detect the situation and
> raise an error. No extension in test or core had the issues.
> 
> diff --git a/mercurial/registrar.py b/mercurial/registrar.py
> --- a/mercurial/registrar.py
> +++ b/mercurial/registrar.py
> @@ -8,6 +8,7 @@
>  from __future__ import absolute_import
>  
>  from . import (
> +    error,
>      pycompat,
>      util,
>  )
> @@ -55,6 +56,9 @@ class _funcregistrarbase(object):
>              func._origdoc = doc
>              func.__doc__ = self._formatdoc(decl, doc)
>  
> +        if name in self._table:
> +            raise error.ProgrammingError('duplicate registration for name %s',
> +                                         name)

It should be '% name'.

I slightly prefer raising the error before modifying the func object, but that
would make little difference since ProgrammingError isn't a recoverable error.

>          self._table[name] = func
>          self._extrasetup(name, func, *args, **kwargs)
Pierre-Yves David - Dec. 19, 2016, 7:51 a.m.
On 12/18/2016 08:24 AM, Yuya Nishihara wrote:
> On Fri, 16 Dec 2016 20:31:29 +0100, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1481545965 -3600
>> #      Mon Dec 12 13:32:45 2016 +0100
>> # Node ID f3479f74af0d2b21c89e906c503f4cddf4b3c39b
>> # Parent  182cacaa4c32330c0466b7111a75d060830783e8
>> registrar: raise a programming error on duplicated registering
>>
>> Previous, registering different object with the same name would silently
>> overwrite the first value with the second one. We now detect the situation and
>> raise an error. No extension in test or core had the issues.
>>
>> diff --git a/mercurial/registrar.py b/mercurial/registrar.py
>> --- a/mercurial/registrar.py
>> +++ b/mercurial/registrar.py
>> @@ -8,6 +8,7 @@
>>  from __future__ import absolute_import
>>
>>  from . import (
>> +    error,
>>      pycompat,
>>      util,
>>  )
>> @@ -55,6 +56,9 @@ class _funcregistrarbase(object):
>>              func._origdoc = doc
>>              func.__doc__ = self._formatdoc(decl, doc)
>>
>> +        if name in self._table:
>> +            raise error.ProgrammingError('duplicate registration for name %s',
>> +                                         name)
>
> It should be '% name'.

I'm not sure? We usually have the offending entry at the end because it 
has variable length. What do you think?
(I made a minor change in the new version,
'duplicate registration for name: "%s"')


> I slightly prefer raising the error before modifying the func object, but that
> would make little difference since ProgrammingError isn't a recoverable error.

Ha!, very good catch. I pushed an updated version here, if you want to 
look at/take it.

https://www.mercurial-scm.org/repo/users/marmoute/mercurial/rev/b52e8a4f4c0f

>>          self._table[name] = func
>>          self._extrasetup(name, func, *args, **kwargs)
Yuya Nishihara - Dec. 19, 2016, 2:09 p.m.
On Mon, 19 Dec 2016 08:51:49 +0100, Pierre-Yves David wrote:
> On 12/18/2016 08:24 AM, Yuya Nishihara wrote:
> > On Fri, 16 Dec 2016 20:31:29 +0100, Pierre-Yves David wrote:
> >> # HG changeset patch
> >> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> >> # Date 1481545965 -3600
> >> #      Mon Dec 12 13:32:45 2016 +0100
> >> # Node ID f3479f74af0d2b21c89e906c503f4cddf4b3c39b
> >> # Parent  182cacaa4c32330c0466b7111a75d060830783e8
> >> registrar: raise a programming error on duplicated registering
> >>
> >> Previous, registering different object with the same name would silently
> >> overwrite the first value with the second one. We now detect the situation and
> >> raise an error. No extension in test or core had the issues.
> >>
> >> diff --git a/mercurial/registrar.py b/mercurial/registrar.py
> >> --- a/mercurial/registrar.py
> >> +++ b/mercurial/registrar.py
> >> @@ -8,6 +8,7 @@
> >>  from __future__ import absolute_import
> >>
> >>  from . import (
> >> +    error,
> >>      pycompat,
> >>      util,
> >>  )
> >> @@ -55,6 +56,9 @@ class _funcregistrarbase(object):
> >>              func._origdoc = doc
> >>              func.__doc__ = self._formatdoc(decl, doc)
> >>
> >> +        if name in self._table:
> >> +            raise error.ProgrammingError('duplicate registration for name %s',
> >> +                                         name)
> >
> > It should be '% name'.
> 
> I'm not sure? We usually have the offending entry at the end because it 
> has variable length. What do you think?
> (I made a minor change in the new version,
> 'duplicate registration for name: "%s"')

I meant s/,/%/.

> > I slightly prefer raising the error before modifying the func object, but that
> > would make little difference since ProgrammingError isn't a recoverable error.
> 
> Ha!, very good catch. I pushed an updated version here, if you want to 
> look at/take it.
> 
> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/rev/b52e8a4f4c0f

I'll take this, thanks.
Pierre-Yves David - Dec. 19, 2016, 2:49 p.m.
On 12/19/2016 03:09 PM, Yuya Nishihara wrote:
> On Mon, 19 Dec 2016 08:51:49 +0100, Pierre-Yves David wrote:
>> On 12/18/2016 08:24 AM, Yuya Nishihara wrote:
>>> On Fri, 16 Dec 2016 20:31:29 +0100, Pierre-Yves David wrote:
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>>>> # Date 1481545965 -3600
>>>> #      Mon Dec 12 13:32:45 2016 +0100
>>>> # Node ID f3479f74af0d2b21c89e906c503f4cddf4b3c39b
>>>> # Parent  182cacaa4c32330c0466b7111a75d060830783e8
>>>> registrar: raise a programming error on duplicated registering
>>>>
>>>> Previous, registering different object with the same name would silently
>>>> overwrite the first value with the second one. We now detect the situation and
>>>> raise an error. No extension in test or core had the issues.
>>>>
>>>> diff --git a/mercurial/registrar.py b/mercurial/registrar.py
>>>> --- a/mercurial/registrar.py
>>>> +++ b/mercurial/registrar.py
>>>> @@ -8,6 +8,7 @@
>>>>  from __future__ import absolute_import
>>>>
>>>>  from . import (
>>>> +    error,
>>>>      pycompat,
>>>>      util,
>>>>  )
>>>> @@ -55,6 +56,9 @@ class _funcregistrarbase(object):
>>>>              func._origdoc = doc
>>>>              func.__doc__ = self._formatdoc(decl, doc)
>>>>
>>>> +        if name in self._table:
>>>> +            raise error.ProgrammingError('duplicate registration for name %s',
>>>> +                                         name)
>>>
>>> It should be '% name'.
>>
>> I'm not sure? We usually have the offending entry at the end because it
>> has variable length. What do you think?
>> (I made a minor change in the new version,
>> 'duplicate registration for name: "%s"')
>
> I meant s/,/%/.

Ha right, I've apparently fixed this without paying attention in my 
rework :-)

>>> I slightly prefer raising the error before modifying the func object, but that
>>> would make little difference since ProgrammingError isn't a recoverable error.
>>
>> Ha!, very good catch. I pushed an updated version here, if you want to
>> look at/take it.
>>
>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/rev/b52e8a4f4c0f
>
> I'll take this, thanks.

Thanks!

Patch

diff --git a/mercurial/registrar.py b/mercurial/registrar.py
--- a/mercurial/registrar.py
+++ b/mercurial/registrar.py
@@ -8,6 +8,7 @@ 
 from __future__ import absolute_import
 
 from . import (
+    error,
     pycompat,
     util,
 )
@@ -55,6 +56,9 @@  class _funcregistrarbase(object):
             func._origdoc = doc
             func.__doc__ = self._formatdoc(decl, doc)
 
+        if name in self._table:
+            raise error.ProgrammingError('duplicate registration for name %s',
+                                         name)
         self._table[name] = func
         self._extrasetup(name, func, *args, **kwargs)