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

login
register
mail settings
Submitter Pierre-Yves David
Date June 21, 2017, 8:55 a.m.
Message ID <e04301d9657950913afc.1498035307@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/21590/
State Accepted
Headers show

Comments

Pierre-Yves David - June 21, 2017, 8:55 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@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.)
via Mercurial-devel - June 22, 2017, 6:13 a.m.
On Wed, Jun 21, 2017 at 1:55 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@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.

> +            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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - June 22, 2017, 6:26 a.m.
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@ens-lyon.org> wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@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.

> 
>> +            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@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - June 22, 2017, 6:30 a.m.
On Wed, Jun 21, 2017 at 11:26 PM, Pierre-Yves David
<pierre-yves.david@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@ens-lyon.org> wrote:
>>>
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@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@mercurial-scm.org
>>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
> --
> Pierre-Yves David

Patch

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