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