launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:simplify-module-hacks into launchpad:master.

Commit message:
Don't try to set __module__ on created interfaces

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/382816

zope.interface 5.0.0 makes __module__ read-only.  Instead, set it when creating the interface; while we're at it, use a temporary module rather than stuffing things into lp.testing.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:simplify-module-hacks into launchpad:master.

diff --git a/lib/lp/services/webapp/doc/canonical_url.txt b/lib/lp/services/webapp/doc/canonical_url.txt
index 8a52aa7..1082d4b 100644
--- a/lib/lp/services/webapp/doc/canonical_url.txt
+++ b/lib/lp/services/webapp/doc/canonical_url.txt
@@ -9,34 +9,31 @@ The browser:url directive registers an ICanonicalUrlData adapter.
 In this test, we'll use a URL hierarchy /countries/England/+towns/London
 
 In this test, we'll use interfaces ICountrySet, ICountry and ITown, which we
-will put in lp.testing.
+will put in a temporary module.
 
+    >>> import sys
+    >>> import types
     >>> from lp.services.webapp.interfaces import ICanonicalUrlData
     >>> from zope.interface import Interface, Attribute, implementer
 
+    >>> module = types.ModuleType(factory.getUniqueString().replace('-', '_'))
+    >>> sys.modules[module.__name__] = module
+
     >>> class ICountrySet(Interface):
-    ...     pass
+    ...     __module__ = module.__name__
 
     >>> class ICountry(Interface):
+    ...     __module__ = module.__name__
     ...     name = Attribute('the name of this country')
 
     >>> class ITown(Interface):
     ...     """Dummy interface for use in browser:url tests."""
+    ...     __module__ = module.__name__
     ...     country = Attribute('the country the town is in')
     ...     name = Attribute('the name of this town')
 
-XXX: This is evil, as it changes the hash of the interfaces. It *must* be done
-before they're registered anywhere.
-
-Put the interfaces into lp.testing, ensuring that there
-are not already objects with those names there.
-
-    >>> import lp.testing
     >>> for interface in ICountrySet, ICountry, ITown:
-    ...     name = interface.getName()
-    ...     assert getattr(lp.testing, name, None) is None
-    ...     setattr(lp.testing, name, interface)
-    ...     interface.__module__ = 'lp.testing'
+    ...     setattr(module, interface.getName(), interface)
 
 Add a view for Country/+map.
 
@@ -107,27 +104,27 @@ Configure a browser:url for ITown.  Our first attempt fails because we mistyped
     ...   <include package="zope.component" file="meta.zcml" />
     ...   <include package="lp.services.webapp" file="meta.zcml" />
     ...   <browser:url
-    ...       for="lp.testing.ITown"
-    ...       path_expression="string:+towns/${name}"
+    ...       for="{module_name}.ITown"
+    ...       path_expression="string:+towns/${{name}}"
     ...       attribute_to_parent="countryOopsTypo"
     ...       />
     ... </configure>
-    ... """)
+    ... """.format(module_name=module.__name__))
     Traceback (most recent call last):
     ...
     ZopeXMLConfigurationError: File "<string>", line ...
-        AttributeError: The name "countryOopsTypo" is not in lp.testing.ITown
+        AttributeError: The name "countryOopsTypo" is not in ....ITown
 
     >>> zcmlcontext = xmlconfig.string("""
     ... <configure xmlns:browser="http://namespaces.zope.org/browser";>
     ...   <include package="lp.services.webapp" file="meta.zcml" />
     ...   <browser:url
-    ...       for="lp.testing.ITown"
-    ...       path_expression="string:+towns/${name}"
+    ...       for="{module_name}.ITown"
+    ...       path_expression="string:+towns/${{name}}"
     ...       attribute_to_parent="country"
     ...       />
     ... </configure>
-    ... """)
+    ... """.format(module_name=module.__name__))
 
 Now, there is an ICanonicalUrlData registered for ITown.
 
@@ -143,12 +140,9 @@ Now, there is an ICanonicalUrlData registered for ITown.
 The parent of an object might be accessible via an attribute, or it might
 be a utility.  This is the case for an ICountry object: its parent is the
 ICountrySet.  I need to put the countryset_instance somewhere we can get
-at it from zcml.  I'll put it in lp.testing.
-
-    >>> assert getattr(
-    ...     lp.testing, 'countryset_instance', None) is None
+at it from zcml.  I'll put it in our temporary module.
 
-    >>> lp.testing.countryset_instance = countryset_instance
+    >>> module.countryset_instance = countryset_instance
 
     >>> zcmlcontext = xmlconfig.string("""
     ... <configure
@@ -160,16 +154,16 @@ at it from zcml.  I'll put it in lp.testing.
     ...       <include file="meta.zcml" />
     ...   </configure>
     ...   <utility
-    ...       provides="lp.testing.ICountrySet"
-    ...       component="lp.testing.countryset_instance"
+    ...       provides="{module_name}.ICountrySet"
+    ...       component="{module_name}.countryset_instance"
     ...       />
     ...   <browser:url
-    ...       for="lp.testing.ICountry"
+    ...       for="{module_name}.ICountry"
     ...       path_expression="name"
-    ...       parent_utility="lp.testing.ICountrySet"
+    ...       parent_utility="{module_name}.ICountrySet"
     ...       />
     ... </configure>
-    ... """)
+    ... """.format(module_name=module.__name__))
 
 Now, there is an ICanonicalUrlData registered for ICountry.
 
@@ -208,19 +202,19 @@ an adapter.
     ...         return getUtility(ILaunchpadRoot)
 
 The CountrySetUrl class needs to be accessible from zcml.  So, we put it
-in lp.testing.
+in our temporary module.
 
-    >>> lp.testing.CountrySetUrl = CountrySetUrl
+    >>> module.CountrySetUrl = CountrySetUrl
 
     >>> zcmlcontext = xmlconfig.string("""
     ... <configure xmlns:browser="http://namespaces.zope.org/browser";>
     ...   <include package="lp.services.webapp" file="meta.zcml" />
     ...   <browser:url
-    ...       for="lp.testing.ICountrySet"
-    ...       urldata="lp.testing.CountrySetUrl"
+    ...       for="{module_name}.ICountrySet"
+    ...       urldata="{module_name}.CountrySetUrl"
     ...       />
     ... </configure>
-    ... """)
+    ... """.format(module_name=module.__name__))
 
 Now, there is an ICanonicalUrlData registered for ICountrySet.
 
@@ -438,17 +432,17 @@ And if the configuration does provide a rootsite:
     ...   <include package="zope.component" file="meta.zcml" />
     ...   <include package="lp.services.webapp" file="meta.zcml" />
     ...   <utility
-    ...       provides="lp.testing.ICountrySet"
-    ...       component="lp.testing.countryset_instance"
+    ...       provides="{module_name}.ICountrySet"
+    ...       component="{module_name}.countryset_instance"
     ...       />
     ...   <browser:url
-    ...       for="lp.testing.ICountry"
+    ...       for="{module_name}.ICountry"
     ...       path_expression="name"
-    ...       parent_utility="lp.testing.ICountrySet"
+    ...       parent_utility="{module_name}.ICountrySet"
     ...       rootsite="bugs"
     ...       />
     ... </configure>
-    ... """)
+    ... """.format(module_name=module.__name__))
 
     >>> canonical_url(country_instance)
     u'http://bugs.launchpad.test/countries/England'
@@ -551,10 +545,7 @@ only the relative local path returned.
 
 == The end ==
 
-We've finished with our interfaces and utility component, so remove them from
-lp.testing.
-
-    >>> for name in ['ICountrySet', 'ICountry', 'ITown', 'countryset_instance',
-    ...              'CountrySetUrl']:
-    ...     delattr(lp.testing, name)
+We've finished with our interfaces and utility component, so remove the
+temporary module.
 
+    >>> del sys.modules[module.__name__]
diff --git a/lib/lp/services/webapp/doc/menus.txt b/lib/lp/services/webapp/doc/menus.txt
index d29f08e..e94d670 100644
--- a/lib/lp/services/webapp/doc/menus.txt
+++ b/lib/lp/services/webapp/doc/menus.txt
@@ -546,30 +546,27 @@ they are as we expect.
 
 == Registering menus in ZCML ==
 
-First, we define a couple of interfaces, and put them in the
-lp.testing module.
+First, we define a couple of interfaces, and put them in a temporary module.
+
+    >>> import sys
+    >>> import types
+
+    >>> module = types.ModuleType(factory.getUniqueString().replace('-', '_'))
+    >>> sys.modules[module.__name__] = module
 
     >>> class IThingHavingFacets(Interface):
-    ...     pass
-    >>> import lp.testing
-    >>> getattr(lp.testing, 'IThingHavingFacets', None) is None
-    True
+    ...     __module__ = module.__name__
 
-    >>> lp.testing.IThingHavingFacets = IThingHavingFacets
-    >>> IThingHavingFacets.__module__ = 'lp.testing'
+    >>> module.IThingHavingFacets = IThingHavingFacets
 
     >>> class IThingHavingMenus(Interface):
-    ...     pass
-    >>> import lp.testing
-    >>> getattr(lp.testing, 'IThingHavingMenus', None) is None
-    True
+    ...     __module__ = module.__name__
 
-    >>> lp.testing.IThingHavingMenus = IThingHavingMenus
-    >>> IThingHavingMenus.__module__ = 'lp.testing'
+    >>> module.IThingHavingMenus = IThingHavingMenus
 
 Next, we define a FacetMenu subclass to be used for IThingHavingFacets,
 using a usedfor class attribute to say what interface it is to be
-registered for, and put it too in the lp.testing module.
+registered for, and put it too in our temporary module.
 
     >>> class FacetsForThing(Facets):
     ...     usedfor = IThingHavingFacets
@@ -585,10 +582,7 @@ registered for, and put it too in the lp.testing module.
     ...             summary = self.request.method
     ...         return Link(target, text, summary=summary)
 
-    >>> getattr(lp.testing, 'FacetsForThing', None) is None
-    True
-
-    >>> lp.testing.FacetsForThing = FacetsForThing
+    >>> module.FacetsForThing = FacetsForThing
 
 And likewise for an application menu registered for IThingHavingMenus.
 
@@ -596,20 +590,14 @@ And likewise for an application menu registered for IThingHavingMenus.
     ...     usedfor = IThingHavingMenus
     ...     facet = 'foo'
 
-    >>> getattr(lp.testing, 'FooMenuForThing', None) is None
-    True
-
-    >>> lp.testing.FooMenuForThing = FooMenuForThing
+    >>> module.FooMenuForThing = FooMenuForThing
 
 We do the same for a context menu.
 
     >>> class ContextMenuForThing(MyContextMenu):
     ...     usedfor = IThingHavingMenus
 
-    >>> print getattr(lp.testing, 'ContextMenuForThing', None)
-    None
-
-    >>> lp.testing.ContextMenuForThing = ContextMenuForThing
+    >>> module.ContextMenuForThing = ContextMenuForThing
 
 Now, check that we have no IFacetMenu adapter for an IThingHavingFacets
 object.
@@ -641,11 +629,11 @@ Same for an IContextMenu adapter.
     ... <configure xmlns:browser="http://namespaces.zope.org/browser";>
     ...   <include file="lib/lp/services/webapp/meta.zcml" />
     ...   <browser:menus
-    ...       module="lp.testing"
+    ...       module="{module_name}"
     ...       classes="FacetsForThing FooMenuForThing ContextMenuForThing"
     ...       />
     ... </configure>
-    ... """)
+    ... """.format(module_name=module.__name__))
 
     >>> menu1 = IFacetMenu(something_with_facets)
     >>> menu1.context = something_with_facets
@@ -885,14 +873,9 @@ render() method.
 
 == Cleaning up ==
 
-We're done testing the zcml, so we can clean up the
-lp.testing module.
+We're done testing the zcml, so we can clean up the temporary module.
 
-    >>> del lp.testing.FacetsForThing
-    >>> del lp.testing.FooMenuForThing
-    >>> del lp.testing.ContextMenuForThing
-    >>> del lp.testing.IThingHavingFacets
-    >>> del lp.testing.IThingHavingMenus
+    >>> del sys.modules[module.__name__]
 
 
 == The enabled_with_permission function decorator ==