launchpad-reviewers team mailing list archive

[Andrey Fedoseev <mp+426693@xxxxxxxxxxxxxxxxxx>]

Andrey Fedoseev has proposed merging ~andrey-fedoseev/launchpad:remove-launchpad-object-factory into launchpad:master.

Commit message:
Remove `LaunchpadObjectFactory` wrapper


Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~andrey-fedoseev/launchpad/+git/launchpad/+merge/426693

This wrapper is supposed to be used to produce warning message
whenever a factory method creates an object which isn't wrapped
in security proxy. However, this is enabled only when
LP_PROXY_WARNINGS=1 is set, and no one seems to be using it.

The wrapper implementation hides the list of available methods
of the underlying factory class which prevents code analysis,
so we decided to remove the wrapper.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~andrey-fedoseev/launchpad:remove-launchpad-object-factory into launchpad:master.

diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index 61c3b2d..b6e7442 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -535,7 +535,7 @@ class ObjectFactory(metaclass=AutoDecorate(default_master_store)):
         return epoch + timedelta(minutes=self.getUniqueInteger())
 
 
-class BareLaunchpadObjectFactory(ObjectFactory):
+class LaunchpadObjectFactory(ObjectFactory):
     """Factory methods for creating Launchpad objects.
 
     All the factory methods should be callable with no parameters.
@@ -5521,15 +5521,6 @@ def is_security_proxied_or_harmless(obj):
     return False
 
 
-class UnproxiedFactoryMethodWarning(UserWarning):
-    """Raised when someone calls an unproxied factory method."""
-
-    def __init__(self, method_name):
-        super().__init__(
-            "PLEASE FIX: LaunchpadObjectFactory.%s returns an "
-            "unproxied object." % (method_name, ))
-
-
 class ShouldThisBeUsingRemoveSecurityProxy(UserWarning):
     """Raised when there is a potentially bad call to removeSecurityProxy."""
 
@@ -5540,42 +5531,6 @@ class ShouldThisBeUsingRemoveSecurityProxy(UserWarning):
         super().__init__(message)
 
 
-class LaunchpadObjectFactory:
-    """A wrapper around `BareLaunchpadObjectFactory`.
-
-    Ensure that each object created by a `BareLaunchpadObjectFactory` method
-    is either of a simple Python type or is security proxied.
-
-    A warning message is printed to stderr if a factory method creates
-    an object without a security proxy.
-
-    Whereever you see such a warning: fix it!
-    """
-
-    def __init__(self):
-        self._factory = BareLaunchpadObjectFactory()
-
-    def __getattr__(self, name):
-        attr = getattr(self._factory, name)
-        if os.environ.get('LP_PROXY_WARNINGS') == '1' and callable(attr):
-
-            def guarded_method(*args, **kw):
-                result = attr(*args, **kw)
-                if not is_security_proxied_or_harmless(result):
-                    warnings.warn(
-                        UnproxiedFactoryMethodWarning(name), stacklevel=1)
-                return result
-            return guarded_method
-        else:
-            return attr
-
-    def __dir__(self):
-        """Enumerate the attributes and methods of the wrapped object factory.
-
-        This is especially useful for interactive users."""
-        return dir(self._factory)
-
-
 def remove_security_proxy_and_shout_at_engineer(obj):
     """Remove an object's security proxy and print a warning.
 
diff --git a/lib/lp/testing/tests/test_factory.py b/lib/lp/testing/tests/test_factory.py
index 4953455..1256e7b 100644
--- a/lib/lp/testing/tests/test_factory.py
+++ b/lib/lp/testing/tests/test_factory.py
@@ -644,14 +644,6 @@ class TestFactory(TestCaseWithFactory):
             sequence='2000-1234', cvestate=CveStatus.DEPRECATED)
         self.assertEqual(CveStatus.DEPRECATED, cve.status)
 
-    # dir() support.
-    def test_dir(self):
-        # LaunchpadObjectFactory supports dir() even though all of its
-        # attributes are pseudo-attributes.
-        self.assertEqual(
-            dir(self.factory._factory),
-            dir(self.factory))
-
     def test_getUniqueString_with_prefix(self):
         s = self.factory.getUniqueString("with-my-prefix")
         self.assertTrue(s.startswith("with-my-prefix"))