[PATCH 6 of 8] configitems: issue a devel warning when overriding default config

Martin von Zweigbergk martinvonz at google.com
Thu Jun 22 02:30:43 EDT 2017


On Wed, Jun 21, 2017 at 11:26 PM, Pierre-Yves David
<pierre-yves.david at ens-lyon.org> wrote:
>
>
> On 06/22/2017 08:13 AM, Martin von Zweigbergk wrote:
>>
>> On Wed, Jun 21, 2017 at 1:55 AM, Pierre-Yves David
>> <pierre-yves.david at ens-lyon.org> wrote:
>>>
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david at octobus.net>
>>> # Date 1497697683 -7200
>>> #      Sat Jun 17 13:08:03 2017 +0200
>>> # Node ID e04301d9657950913afc147026a67a50a40d3e75
>>> # Parent  26bc51ab6c2c49778e967807ec0b594fd7ac7d2e
>>> # EXP-Topic config.register
>>> # Available At
>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>>> #              hg pull
>>> https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r e04301d96579
>>> configitems: issue a devel warning when overriding default config
>>>
>>> If the option is registered, there is already a default value available
>>> and
>>> passing a new one is at best redundant. So we issue a deprecation warning
>>> in
>>> this case.
>>>
>>> (note: there will be case were the default value will not be as simple as
>>> what
>>> is currently possible. We'll upgrade the configitems code to handle them
>>> in
>>> time.)
>>>
>>> diff --git a/mercurial/ui.py b/mercurial/ui.py
>>> --- a/mercurial/ui.py
>>> +++ b/mercurial/ui.py
>>> @@ -445,11 +445,15 @@ class ui(object):
>>>               if default is _unset:
>>>                   default = None
>>>           else:
>>> +            item = self._knownconfig.get(section, {}).get(name)
>>>               if default is _unset:
>>> -                default = None
>>> -                item = self._knownconfig.get(section, {}).get(name)
>>> -                if item is not None:
>>> -                    default = item.default
>>> +                default = getattr(item, 'default', None)
>>
>>
>> Is the switch to getattr() related ot the rest of the patch? I prefer
>> the previous form, so I'd rather revert the change to getattr() unless
>> it's needed or useful for some reason.
>
>
> It is mostly related to the patch as it simplify the logic around the
> non-registered configitem (get returns None) now item logic is wider.
>
> It allow to replace the following code:
>
>   default = None
>   if item is not None:
>       default = item.default
>
> With:
>     default = getattr(item, 'default', None)
>
> The previous version of the code was not using that form because the code
> block was focusing on "item" retrieval and processing. Now that item is
> retrieved earlier and as a larger life span it made sense to me to use the
> shorter form.
>
> We could do without it if you prefer.

I do. It makes it clearer under what circumstances item won't have a
default (when it is None). I'll change in flight.

>
>
>>
>>> +            elif item is not None:
>>> +                msg = ("specifying a default value for a registered "
>>> +                       "config item: '%s.%s' '%s'")
>>> +                msg %= (section, name, default)
>>> +                self.develwarn(msg, 1, 'warn-config-default')
>>> +
>>>               alternates = [name]
>>>
>>>           for n in alternates:
>>> diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
>>> --- a/tests/test-devel-warnings.t
>>> +++ b/tests/test-devel-warnings.t
>>> @@ -193,4 +193,24 @@ Old style deprecation warning
>>>
>>>     $ HGEMITWARNINGS= hg nouiwarning
>>>
>>> +Test warning on config option access and registration
>>> +
>>> +  $ cat << EOF > ${TESTTMP}/buggyconfig.py
>>> +  > """A small extension that tests our developer warnings for config"""
>>> +  >
>>> +  > from mercurial import registrar
>>> +  >
>>> +  > cmdtable = {}
>>> +  > command = registrar.command(cmdtable)
>>> +  >
>>> +  > @command('buggyconfig')
>>> +  > def cmdbuggyconfig(ui, repo):
>>> +  >     repo.ui.config('ui', 'quiet', False)
>>> +  >     repo.ui.config('ui', 'interactive', None)
>>> +  > EOF
>>> +
>>> +  $ hg --config "extensions.buggyconfig=${TESTTMP}/buggyconfig.py"
>>> buggyconfig
>>> +  devel-warn: specifying a default value for a registered config item:
>>> 'ui.quiet' 'False' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
>>> +  devel-warn: specifying a default value for a registered config item:
>>> 'ui.interactive' 'None' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig)
>>> (glob)
>>> +
>>>     $ cd ..
>>> _______________________________________________
>>> Mercurial-devel mailing list
>>> Mercurial-devel at mercurial-scm.org
>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
> --
> Pierre-Yves David


More information about the Mercurial-devel mailing list