Patchwork [STABLE] configitems: relax warning about unwanted default value

login
register
mail settings
Submitter Yuya Nishihara
Date Oct. 31, 2017, 2:32 p.m.
Message ID <4f00958f5fc9d7ae9046.1509460349@mimosa>
Download mbox | patch
Permalink /patch/25315/
State Accepted
Headers show

Comments

Yuya Nishihara - Oct. 31, 2017, 2:32 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1509457050 -32400
#      Tue Oct 31 22:37:30 2017 +0900
# Branch stable
# Node ID 4f00958f5fc9d7ae90468f6159dff7720d0f2b27
# Parent  5caae380ff90735b1a1841a4f26147bf0f01351b
configitems: relax warning about unwanted default value

The original condition was a bit harsh for extension authors since third-party
extensions need to preserve compatibility with older Mercurial versions, where
no defaults would be loaded from the configtable. So let's silence the warning
if the given default value matches, which should be harmless.
Augie Fackler - Oct. 31, 2017, 2:44 p.m.
> On Oct 31, 2017, at 10:32, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1509457050 -32400
> #      Tue Oct 31 22:37:30 2017 +0900
> # Branch stable
> # Node ID 4f00958f5fc9d7ae90468f6159dff7720d0f2b27
> # Parent  5caae380ff90735b1a1841a4f26147bf0f01351b
> configitems: relax warning about unwanted default value
> 
> The original condition was a bit harsh for extension authors since third-party
> extensions need to preserve compatibility with older Mercurial versions, where
> no defaults would be loaded from the configtable. So let's silence the warning
> if the given default value matches, which should be harmless.
> 
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -471,12 +471,16 @@ class ui(object):
>         return value
> 
>     def _config(self, section, name, default=_unset, untrusted=False):
> -        value = default
> +        value = itemdefault = default
>         item = self._knownconfig.get(section, {}).get(name)
>         alternates = [(section, name)]
> 
>         if item is not None:
>             alternates.extend(item.alias)
> +            if callable(item.default):
> +                itemdefault = item.default()
> +            else:
> +                itemdefault = item.default
>         else:
>             msg = ("accessing unregistered config item: '%s.%s'")
>             msg %= (section, name)
> @@ -490,12 +494,11 @@ class ui(object):
>                 msg = "config item requires an explicit default value: '%s.%s'"
>                 msg %= (section, name)
>                 self.develwarn(msg, 2, 'warn-config-default')
> -            elif callable(item.default):
> -                    value = item.default()
>             else:
> -                value = item.default
> +                value = itemdefault
>         elif (item is not None
> -              and item.default is not configitems.dynamicdefault):
> +              and item.default is not configitems.dynamicdefault
> +              and default != itemdefault):
>             msg = ("specifying a default value for a registered "
>                    "config item: '%s.%s' '%s'")

nit: should I edit the message to say "mismatched default" since that's what we're really complaining about?

>             msg %= (section, name, default)
> 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
> @@ -352,17 +352,21 @@ Test warning on config option access and
>> 
>> configitem('test', 'some', default='foo')
>> configitem('test', 'dynamic', default=configitems.dynamicdefault)
> +  > configitem('test', 'callable', default=list)
>> # overwrite a core config
>> configitem('ui', 'quiet', default=False)
>> configitem('ui', 'interactive', default=None)
>> 
>> @command(b'buggyconfig')
>> def cmdbuggyconfig(ui, repo):
> -  >     repo.ui.config('ui', 'quiet', False)
> -  >     repo.ui.config('ui', 'interactive', None)
> +  >     repo.ui.config('ui', 'quiet', True)
> +  >     repo.ui.config('ui', 'interactive', False)
> +  >     repo.ui.config('test', 'some', 'bar')
>>    repo.ui.config('test', 'some', 'foo')
>>    repo.ui.config('test', 'dynamic', 'some-required-default')
>>    repo.ui.config('test', 'dynamic')
> +  >     repo.ui.config('test', 'callable', [])
> +  >     repo.ui.config('test', 'callable', 'foo')
>>    repo.ui.config('test', 'unregistered')
>>    repo.ui.config('unregistered', 'unregistered')
>> EOF
> @@ -370,10 +374,11 @@ Test warning on config option access and
>   $ hg --config "extensions.buggyconfig=${TESTTMP}/buggyconfig.py" buggyconfig
>   devel-warn: extension 'buggyconfig' overwrite config item 'ui.interactive' at: */mercurial/extensions.py:* (_loadextra) (glob)
>   devel-warn: extension 'buggyconfig' overwrite config item 'ui.quiet' at: */mercurial/extensions.py:* (_loadextra) (glob)
> -  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)
> -  devel-warn: specifying a default value for a registered config item: 'test.some' 'foo' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
> +  devel-warn: specifying a default value for a registered config item: 'ui.quiet' 'True' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
> +  devel-warn: specifying a default value for a registered config item: 'ui.interactive' 'False' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
> +  devel-warn: specifying a default value for a registered config item: 'test.some' 'bar' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
>   devel-warn: config item requires an explicit default value: 'test.dynamic' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
> +  devel-warn: specifying a default value for a registered config item: 'test.callable' 'foo' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
>   devel-warn: accessing unregistered config item: 'test.unregistered' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
>   devel-warn: accessing unregistered config item: 'unregistered.unregistered' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - Oct. 31, 2017, 3:30 p.m.
On Tue, 31 Oct 2017 10:44:42 -0400, Augie Fackler wrote:
> > On Oct 31, 2017, at 10:32, Yuya Nishihara <yuya@tcha.org> wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1509457050 -32400
> > #      Tue Oct 31 22:37:30 2017 +0900
> > # Branch stable
> > # Node ID 4f00958f5fc9d7ae90468f6159dff7720d0f2b27
> > # Parent  5caae380ff90735b1a1841a4f26147bf0f01351b
> > configitems: relax warning about unwanted default value
> > 
> > The original condition was a bit harsh for extension authors since third-party
> > extensions need to preserve compatibility with older Mercurial versions, where
> > no defaults would be loaded from the configtable. So let's silence the warning
> > if the given default value matches, which should be harmless.
> > 
> > diff --git a/mercurial/ui.py b/mercurial/ui.py
> > --- a/mercurial/ui.py
> > +++ b/mercurial/ui.py
> > @@ -471,12 +471,16 @@ class ui(object):
> >         return value
> > 
> >     def _config(self, section, name, default=_unset, untrusted=False):
> > -        value = default
> > +        value = itemdefault = default
> >         item = self._knownconfig.get(section, {}).get(name)
> >         alternates = [(section, name)]
> > 
> >         if item is not None:
> >             alternates.extend(item.alias)
> > +            if callable(item.default):
> > +                itemdefault = item.default()
> > +            else:
> > +                itemdefault = item.default
> >         else:
> >             msg = ("accessing unregistered config item: '%s.%s'")
> >             msg %= (section, name)
> > @@ -490,12 +494,11 @@ class ui(object):
> >                 msg = "config item requires an explicit default value: '%s.%s'"
> >                 msg %= (section, name)
> >                 self.develwarn(msg, 2, 'warn-config-default')
> > -            elif callable(item.default):
> > -                    value = item.default()
> >             else:
> > -                value = item.default
> > +                value = itemdefault
> >         elif (item is not None
> > -              and item.default is not configitems.dynamicdefault):
> > +              and item.default is not configitems.dynamicdefault
> > +              and default != itemdefault):
> >             msg = ("specifying a default value for a registered "
> >                    "config item: '%s.%s' '%s'")
> 
> nit: should I edit the message to say "mismatched default" since that's what we're really complaining about?

I don't care much about that, but "specifying a mismatched default" sounds
more correct. Can you update it in flight?
Augie Fackler - Nov. 1, 2017, 2:55 p.m.
> On Oct 31, 2017, at 11:30, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Tue, 31 Oct 2017 10:44:42 -0400, Augie Fackler wrote:
>>> On Oct 31, 2017, at 10:32, Yuya Nishihara <yuya@tcha.org> wrote:
>>> # HG changeset patch
>>> # User Yuya Nishihara <yuya@tcha.org>
>>> # Date 1509457050 -32400
>>> #      Tue Oct 31 22:37:30 2017 +0900
>>> # Branch stable
>>> # Node ID 4f00958f5fc9d7ae90468f6159dff7720d0f2b27
>>> # Parent  5caae380ff90735b1a1841a4f26147bf0f01351b
>>> configitems: relax warning about unwanted default value
>>> 
>>> The original condition was a bit harsh for extension authors since third-party
>>> extensions need to preserve compatibility with older Mercurial versions, where
>>> no defaults would be loaded from the configtable. So let's silence the warning
>>> if the given default value matches, which should be harmless.
>>> 
>>> diff --git a/mercurial/ui.py b/mercurial/ui.py
>>> --- a/mercurial/ui.py
>>> +++ b/mercurial/ui.py
>>> @@ -471,12 +471,16 @@ class ui(object):
>>>        return value
>>> 
>>>    def _config(self, section, name, default=_unset, untrusted=False):
>>> -        value = default
>>> +        value = itemdefault = default
>>>        item = self._knownconfig.get(section, {}).get(name)
>>>        alternates = [(section, name)]
>>> 
>>>        if item is not None:
>>>            alternates.extend(item.alias)
>>> +            if callable(item.default):
>>> +                itemdefault = item.default()
>>> +            else:
>>> +                itemdefault = item.default
>>>        else:
>>>            msg = ("accessing unregistered config item: '%s.%s'")
>>>            msg %= (section, name)
>>> @@ -490,12 +494,11 @@ class ui(object):
>>>                msg = "config item requires an explicit default value: '%s.%s'"
>>>                msg %= (section, name)
>>>                self.develwarn(msg, 2, 'warn-config-default')
>>> -            elif callable(item.default):
>>> -                    value = item.default()
>>>            else:
>>> -                value = item.default
>>> +                value = itemdefault
>>>        elif (item is not None
>>> -              and item.default is not configitems.dynamicdefault):
>>> +              and item.default is not configitems.dynamicdefault
>>> +              and default != itemdefault):
>>>            msg = ("specifying a default value for a registered "
>>>                   "config item: '%s.%s' '%s'")
>> 
>> nit: should I edit the message to say "mismatched default" since that's what we're really complaining about?
> 
> I don't care much about that, but "specifying a mismatched default" sounds
> more correct. Can you update it in flight?

Queued with that fixed in flight, many thanks.

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -471,12 +471,16 @@  class ui(object):
         return value
 
     def _config(self, section, name, default=_unset, untrusted=False):
-        value = default
+        value = itemdefault = default
         item = self._knownconfig.get(section, {}).get(name)
         alternates = [(section, name)]
 
         if item is not None:
             alternates.extend(item.alias)
+            if callable(item.default):
+                itemdefault = item.default()
+            else:
+                itemdefault = item.default
         else:
             msg = ("accessing unregistered config item: '%s.%s'")
             msg %= (section, name)
@@ -490,12 +494,11 @@  class ui(object):
                 msg = "config item requires an explicit default value: '%s.%s'"
                 msg %= (section, name)
                 self.develwarn(msg, 2, 'warn-config-default')
-            elif callable(item.default):
-                    value = item.default()
             else:
-                value = item.default
+                value = itemdefault
         elif (item is not None
-              and item.default is not configitems.dynamicdefault):
+              and item.default is not configitems.dynamicdefault
+              and default != itemdefault):
             msg = ("specifying a default value for a registered "
                    "config item: '%s.%s' '%s'")
             msg %= (section, name, default)
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
@@ -352,17 +352,21 @@  Test warning on config option access and
   > 
   > configitem('test', 'some', default='foo')
   > configitem('test', 'dynamic', default=configitems.dynamicdefault)
+  > configitem('test', 'callable', default=list)
   > # overwrite a core config
   > configitem('ui', 'quiet', default=False)
   > configitem('ui', 'interactive', default=None)
   > 
   > @command(b'buggyconfig')
   > def cmdbuggyconfig(ui, repo):
-  >     repo.ui.config('ui', 'quiet', False)
-  >     repo.ui.config('ui', 'interactive', None)
+  >     repo.ui.config('ui', 'quiet', True)
+  >     repo.ui.config('ui', 'interactive', False)
+  >     repo.ui.config('test', 'some', 'bar')
   >     repo.ui.config('test', 'some', 'foo')
   >     repo.ui.config('test', 'dynamic', 'some-required-default')
   >     repo.ui.config('test', 'dynamic')
+  >     repo.ui.config('test', 'callable', [])
+  >     repo.ui.config('test', 'callable', 'foo')
   >     repo.ui.config('test', 'unregistered')
   >     repo.ui.config('unregistered', 'unregistered')
   > EOF
@@ -370,10 +374,11 @@  Test warning on config option access and
   $ hg --config "extensions.buggyconfig=${TESTTMP}/buggyconfig.py" buggyconfig
   devel-warn: extension 'buggyconfig' overwrite config item 'ui.interactive' at: */mercurial/extensions.py:* (_loadextra) (glob)
   devel-warn: extension 'buggyconfig' overwrite config item 'ui.quiet' at: */mercurial/extensions.py:* (_loadextra) (glob)
-  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)
-  devel-warn: specifying a default value for a registered config item: 'test.some' 'foo' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
+  devel-warn: specifying a default value for a registered config item: 'ui.quiet' 'True' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
+  devel-warn: specifying a default value for a registered config item: 'ui.interactive' 'False' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
+  devel-warn: specifying a default value for a registered config item: 'test.some' 'bar' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
   devel-warn: config item requires an explicit default value: 'test.dynamic' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
+  devel-warn: specifying a default value for a registered config item: 'test.callable' 'foo' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
   devel-warn: accessing unregistered config item: 'test.unregistered' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)
   devel-warn: accessing unregistered config item: 'unregistered.unregistered' at: $TESTTMP/buggyconfig.py:* (cmdbuggyconfig) (glob)