Patchwork [6,of,6,RFC] extensions: show deprecation warning for the use of cmdutil.command

login
register
mail settings
Submitter Yuya Nishihara
Date May 13, 2017, 9:57 a.m.
Message ID <1b5d23403828e246d898.1494669457@mimosa>
Download mbox | patch
Permalink /patch/20597/
State Accepted
Headers show

Comments

Yuya Nishihara - May 13, 2017, 9:57 a.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1452349492 -32400
#      Sat Jan 09 23:24:52 2016 +0900
# Node ID 1b5d23403828e246d89816c4826a25a7de586cee
# Parent  484b2808830a2a390d9e1128c6e88d9249097fa6
extensions: show deprecation warning for the use of cmdutil.command

This is RFC. Do we really want to remove cmdutil.command?
Gregory Szorc - May 13, 2017, 6 p.m.
On Sat, May 13, 2017 at 2:57 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1452349492 -32400
> #      Sat Jan 09 23:24:52 2016 +0900
> # Node ID 1b5d23403828e246d89816c4826a25a7de586cee
> # Parent  484b2808830a2a390d9e1128c6e88d9249097fa6
> extensions: show deprecation warning for the use of cmdutil.command
>
> This is RFC. Do we really want to remove cmdutil.command?
>

I'm in favor of this. But I think it should be deprecated for multiple
releases to give extensions time to transition. The reason is this is one
of the oldest and most used APIs in extensions. When we remove it, we'll
break a lot of extensions. But simply marking it as deprecated has no
impact unless developer warnings are on. So it shouldn't be too
controversial to start the process.


>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -3334,7 +3334,10 @@ def _performrevert(repo, parents, ctx, a
>          if f in copied:
>              repo.dirstate.copy(copied[f], f)
>
> -command = registrar.command
> +class command(registrar.command):
> +    def _doregister(self, func, name, *args, **kwargs):
> +        func._deprecatedregistrar = True  # flag for deprecwarn in
> extensions.py
> +        return super(command, self)._doregister(func, name, *args,
> **kwargs)
>
>  # a list of (ui, repo, otherpeer, opts, missing) functions called by
>  # commands.outgoing.  "missing" is "missing" of the result of
> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -121,10 +121,13 @@ def _reportimporterror(ui, err, failed,
>  # attributes set by registrar.command
>  _cmdfuncattrs = ('norepo', 'optionalrepo', 'inferrepo')
>
> -def _validatecmdtable(cmdtable):
> +def _validatecmdtable(ui, cmdtable):
>      """Check if extension commands have required attributes"""
>      for c, e in cmdtable.iteritems():
>          f = e[0]
> +        if getattr(f, '_deprecatedregistrar', False):
> +            ui.deprecwarn("cmdutil.command is deprecated, use "
> +                          "registrar.command to register '%s'" % c, '4.3')
>          missing = [a for a in _cmdfuncattrs if not util.safehasattr(f, a)]
>          if not missing:
>              continue
> @@ -152,7 +155,7 @@ def load(ui, name, path):
>          ui.warn(_('(third party extension %s requires version %s or newer
> '
>                    'of Mercurial; disabling)\n') % (shortname, minver))
>          return
> -    _validatecmdtable(getattr(mod, 'cmdtable', {}))
> +    _validatecmdtable(ui, getattr(mod, 'cmdtable', {}))
>
>      _extensions[shortname] = mod
>      _order.append(shortname)
> diff --git a/tests/test-extension.t b/tests/test-extension.t
> --- a/tests/test-extension.t
> +++ b/tests/test-extension.t
> @@ -1588,4 +1588,19 @@ Test synopsis and docstring extending
>    $ hg help bookmarks | grep GREPME
>    hg bookmarks [OPTIONS]... [NAME]... GREPME [--foo] [-x]
>        GREPME make sure that this is in the help!
> +  $ cd ..
>
> +Show deprecation warning for the use of cmdutil.command
> +
> +  $ cat > nonregistrar.py <<EOF
> +  > from mercurial import cmdutil
> +  > cmdtable = {}
> +  > command = cmdutil.command(cmdtable)
> +  > @command('foo', [], norepo=True)
> +  > def foo(ui):
> +  >     pass
> +  > EOF
> +
> +  $ hg --config extensions.nonregistrar=`pwd`/nonregistrar.py version >
> /dev/null
> +  devel-warn: cmdutil.command is deprecated, use registrar.command to
> register 'foo'
> +  (compatibility will be dropped after Mercurial-4.3, update your code.)
> * (glob)
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Yuya Nishihara - May 14, 2017, 3:11 a.m.
On Sat, 13 May 2017 11:00:02 -0700, Gregory Szorc wrote:
> On Sat, May 13, 2017 at 2:57 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@tcha.org>
> > # Date 1452349492 -32400
> > #      Sat Jan 09 23:24:52 2016 +0900
> > # Node ID 1b5d23403828e246d89816c4826a25a7de586cee
> > # Parent  484b2808830a2a390d9e1128c6e88d9249097fa6
> > extensions: show deprecation warning for the use of cmdutil.command
> >
> > This is RFC. Do we really want to remove cmdutil.command?
> >
> 
> I'm in favor of this. But I think it should be deprecated for multiple
> releases to give extensions time to transition. The reason is this is one
> of the oldest and most used APIs in extensions. When we remove it, we'll
> break a lot of extensions. But simply marking it as deprecated has no
> impact unless developer warnings are on. So it shouldn't be too
> controversial to start the process.

How about setting the target to 4.2 + 2years = 5.0 for now? I don't want
to add version=None support to ui.deprecwarn() API because it would be
so convenient for developers to abuse.
Augie Fackler - May 15, 2017, 5:08 p.m.
On Sun, May 14, 2017 at 12:11:53PM +0900, Yuya Nishihara wrote:
> On Sat, 13 May 2017 11:00:02 -0700, Gregory Szorc wrote:
> > On Sat, May 13, 2017 at 2:57 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > > # HG changeset patch
> > > # User Yuya Nishihara <yuya@tcha.org>
> > > # Date 1452349492 -32400
> > > #      Sat Jan 09 23:24:52 2016 +0900
> > > # Node ID 1b5d23403828e246d89816c4826a25a7de586cee
> > > # Parent  484b2808830a2a390d9e1128c6e88d9249097fa6
> > > extensions: show deprecation warning for the use of cmdutil.command
> > >
> > > This is RFC. Do we really want to remove cmdutil.command?
> > >
> >
> > I'm in favor of this. But I think it should be deprecated for multiple
> > releases to give extensions time to transition. The reason is this is one
> > of the oldest and most used APIs in extensions. When we remove it, we'll
> > break a lot of extensions. But simply marking it as deprecated has no
> > impact unless developer warnings are on. So it shouldn't be too
> > controversial to start the process.
>
> How about setting the target to 4.2 + 2years = 5.0 for now? I don't want
> to add version=None support to ui.deprecwarn() API because it would be
> so convenient for developers to abuse.

2 years is a stinking long time. Could we go with 4.6 at the latest?

(Agree that this is pretty high impact churn that merits some special
attention, but I'm skeptical that there will be a difference between a
year and two years for the deprecation window.)

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - May 16, 2017, 2:42 p.m.
On Mon, 15 May 2017 13:08:15 -0400, Augie Fackler wrote:
> On Sun, May 14, 2017 at 12:11:53PM +0900, Yuya Nishihara wrote:
> > On Sat, 13 May 2017 11:00:02 -0700, Gregory Szorc wrote:
> > > On Sat, May 13, 2017 at 2:57 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > > > # HG changeset patch
> > > > # User Yuya Nishihara <yuya@tcha.org>
> > > > # Date 1452349492 -32400
> > > > #      Sat Jan 09 23:24:52 2016 +0900
> > > > # Node ID 1b5d23403828e246d89816c4826a25a7de586cee
> > > > # Parent  484b2808830a2a390d9e1128c6e88d9249097fa6
> > > > extensions: show deprecation warning for the use of cmdutil.command
> > > >
> > > > This is RFC. Do we really want to remove cmdutil.command?
> > > >
> > >
> > > I'm in favor of this. But I think it should be deprecated for multiple
> > > releases to give extensions time to transition. The reason is this is one
> > > of the oldest and most used APIs in extensions. When we remove it, we'll
> > > break a lot of extensions. But simply marking it as deprecated has no
> > > impact unless developer warnings are on. So it shouldn't be too
> > > controversial to start the process.
> >
> > How about setting the target to 4.2 + 2years = 5.0 for now? I don't want
> > to add version=None support to ui.deprecwarn() API because it would be
> > so convenient for developers to abuse.
> 
> 2 years is a stinking long time. Could we go with 4.6 at the latest?
> 
> (Agree that this is pretty high impact churn that merits some special
> attention, but I'm skeptical that there will be a difference between a
> year and two years for the deprecation window.)

Sure. I'll send V2 with version='4.6' when the first half get queued. We
can extend the limit if that really matters.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -3334,7 +3334,10 @@  def _performrevert(repo, parents, ctx, a
         if f in copied:
             repo.dirstate.copy(copied[f], f)
 
-command = registrar.command
+class command(registrar.command):
+    def _doregister(self, func, name, *args, **kwargs):
+        func._deprecatedregistrar = True  # flag for deprecwarn in extensions.py
+        return super(command, self)._doregister(func, name, *args, **kwargs)
 
 # a list of (ui, repo, otherpeer, opts, missing) functions called by
 # commands.outgoing.  "missing" is "missing" of the result of
diff --git a/mercurial/extensions.py b/mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -121,10 +121,13 @@  def _reportimporterror(ui, err, failed, 
 # attributes set by registrar.command
 _cmdfuncattrs = ('norepo', 'optionalrepo', 'inferrepo')
 
-def _validatecmdtable(cmdtable):
+def _validatecmdtable(ui, cmdtable):
     """Check if extension commands have required attributes"""
     for c, e in cmdtable.iteritems():
         f = e[0]
+        if getattr(f, '_deprecatedregistrar', False):
+            ui.deprecwarn("cmdutil.command is deprecated, use "
+                          "registrar.command to register '%s'" % c, '4.3')
         missing = [a for a in _cmdfuncattrs if not util.safehasattr(f, a)]
         if not missing:
             continue
@@ -152,7 +155,7 @@  def load(ui, name, path):
         ui.warn(_('(third party extension %s requires version %s or newer '
                   'of Mercurial; disabling)\n') % (shortname, minver))
         return
-    _validatecmdtable(getattr(mod, 'cmdtable', {}))
+    _validatecmdtable(ui, getattr(mod, 'cmdtable', {}))
 
     _extensions[shortname] = mod
     _order.append(shortname)
diff --git a/tests/test-extension.t b/tests/test-extension.t
--- a/tests/test-extension.t
+++ b/tests/test-extension.t
@@ -1588,4 +1588,19 @@  Test synopsis and docstring extending
   $ hg help bookmarks | grep GREPME
   hg bookmarks [OPTIONS]... [NAME]... GREPME [--foo] [-x]
       GREPME make sure that this is in the help!
+  $ cd ..
 
+Show deprecation warning for the use of cmdutil.command
+
+  $ cat > nonregistrar.py <<EOF
+  > from mercurial import cmdutil
+  > cmdtable = {}
+  > command = cmdutil.command(cmdtable)
+  > @command('foo', [], norepo=True)
+  > def foo(ui):
+  >     pass
+  > EOF
+
+  $ hg --config extensions.nonregistrar=`pwd`/nonregistrar.py version > /dev/null
+  devel-warn: cmdutil.command is deprecated, use registrar.command to register 'foo'
+  (compatibility will be dropped after Mercurial-4.3, update your code.) * (glob)