Patchwork exthelper: drop the addattr() decorator

login
register
mail settings
Submitter Matt Harbison
Date Jan. 17, 2019, 5:19 a.m.
Message ID <73b630278375d5113777.1547702373@Envy>
Download mbox | patch
Permalink /patch/37845/
State Accepted
Headers show

Comments

Matt Harbison - Jan. 17, 2019, 5:19 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1547702160 18000
#      Thu Jan 17 00:16:00 2019 -0500
# Node ID 73b630278375d51137772d30fa93f9fe009441de
# Parent  4c0d4bbdc39537d00c764b99e88f22e41303cf42
exthelper: drop the addattr() decorator

Yuya pointed out that this goes against the typical advice to not add attributes
to classes[1].  The evolve extension still uses this a handful of times, so
maybe it should be brought back in the future if a general use is found.  But it
isn't nice to have a new helper API that can lead to easy problems.

[1] https://www.mercurial-scm.org/pipermail/mercurial-devel/2018-December/126330.html
Yuya Nishihara - Jan. 17, 2019, 11:47 a.m.
On Thu, 17 Jan 2019 00:19:33 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1547702160 18000
> #      Thu Jan 17 00:16:00 2019 -0500
> # Node ID 73b630278375d51137772d30fa93f9fe009441de
> # Parent  4c0d4bbdc39537d00c764b99e88f22e41303cf42
> exthelper: drop the addattr() decorator

Queued, thanks.

Patch

diff --git a/hgext/lfs/__init__.py b/hgext/lfs/__init__.py
--- a/hgext/lfs/__init__.py
+++ b/hgext/lfs/__init__.py
@@ -130,6 +130,7 @@  from mercurial.i18n import _
 
 from mercurial import (
     config,
+    context,
     error,
     exchange,
     extensions,
@@ -329,6 +330,8 @@  def _resolverevlogstorevfsoptions(orig, 
 def _extsetup(ui):
     wrapfilelog(filelog.filelog)
 
+    context.basefilectx.islfs = wrapper.filectxislfs
+
     scmutil.fileprefetchhooks.add('lfs', wrapper._prefetchfiles)
 
     # Make bundle choose changegroup3 instead of changegroup2. This affects
diff --git a/hgext/lfs/wrapper.py b/hgext/lfs/wrapper.py
--- a/hgext/lfs/wrapper.py
+++ b/hgext/lfs/wrapper.py
@@ -208,7 +208,6 @@  def filectxisbinary(orig, self):
         return bool(int(metadata.get('x-is-binary', 1)))
     return orig(self)
 
-@eh.addattr(context.basefilectx, 'islfs')
 def filectxislfs(self):
     return _islfs(self.filelog(), self.filenode())
 
diff --git a/mercurial/exthelper.py b/mercurial/exthelper.py
--- a/mercurial/exthelper.py
+++ b/mercurial/exthelper.py
@@ -82,7 +82,6 @@  class exthelper(object):
         self._commandwrappers = []
         self._extcommandwrappers = []
         self._functionwrappers = []
-        self._duckpunchers = []
         self.cmdtable = {}
         self.command = registrar.command(self.cmdtable)
         self.configtable = {}
@@ -102,7 +101,6 @@  class exthelper(object):
         self._commandwrappers.extend(other._commandwrappers)
         self._extcommandwrappers.extend(other._extcommandwrappers)
         self._functionwrappers.extend(other._functionwrappers)
-        self._duckpunchers.extend(other._duckpunchers)
         self.cmdtable.update(other.cmdtable)
         for section, items in other.configtable.iteritems():
             if section in self.configtable:
@@ -129,8 +127,6 @@  class exthelper(object):
         - Setup of pre-* and post-* hooks
         - pushkey setup
         """
-        for cont, funcname, func in self._duckpunchers:
-            setattr(cont, funcname, func)
         for command, wrapper, opts in self._commandwrappers:
             entry = extensions.wrapcommand(commands.table, command, wrapper)
             if opts:
@@ -302,29 +298,3 @@  class exthelper(object):
             self._functionwrappers.append((container, funcname, wrapper))
             return wrapper
         return dec
-
-    def addattr(self, container, funcname):
-        """Decorated function is to be added to the container
-
-        This function takes two arguments, the container and the name of the
-        function to wrap. The wrapping is performed during `uisetup`.
-
-        Adding attributes to a container like this is discouraged, because the
-        container modification is visible even in repositories that do not
-        have the extension loaded.  Therefore, care must be taken that the
-        function doesn't make assumptions that the extension was loaded for the
-        current repository.  For `ui` and `repo` instances, a better option is
-        to subclass the instance in `uipopulate` and `reposetup` respectively.
-
-        https://www.mercurial-scm.org/wiki/WritingExtensions
-
-        example::
-
-            @eh.addattr(context.changectx, 'babar')
-            def babar(ctx):
-                return 'babar' in ctx.description
-        """
-        def dec(func):
-            self._duckpunchers.append((container, funcname, func))
-            return func
-        return dec