launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #22754
[Merge] lp:~cjwatson/launchpad/traversal-no-class-advice into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/traversal-no-class-advice into lp:launchpad.
Commit message:
Refactor traversal helpers to avoid using Zope class advice.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/traversal-no-class-advice/+merge/349625
Zope class advice doesn't work in Python 2, and decorators seem somewhat more Pythonic anyway. We just need to store the information in method attributes rather than class attributes.
I considered retaining the two-argument form of redirection as a class decorator, but rewriting the call sites was trivial enough that it wasn't worth it.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/traversal-no-class-advice into lp:launchpad.
=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py 2017-04-12 15:34:45 +0000
+++ lib/lp/bugs/browser/bugtask.py 2018-07-14 21:01:50 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""IBugTask-related browser views."""
@@ -308,11 +308,9 @@
class BugTargetTraversalMixin:
"""Mix-in in class that provides .../+bug/NNN traversal."""
- redirection('+bug', '+bugs')
-
@stepthrough('+bug')
def traverse_bug(self, name):
- """Traverses +bug portions of URLs"""
+ """Traverses +bug portions of URLs."""
return self._get_task_for_context(name)
def _get_task_for_context(self, name):
@@ -350,12 +348,17 @@
# we raise NotFound error. eg +delete or +edit etc.
# Otherwise we are simply navigating to a non-existent task and so we
# redirect to one that exists.
- travseral_stack = self.request.getTraversalStack()
- if len(travseral_stack) > 0:
+ traversal_stack = self.request.getTraversalStack()
+ if len(traversal_stack) > 0:
raise NotFoundError
return self.redirectSubTree(
canonical_url(bug.default_bugtask, request=self.request))
+ @redirection('+bug')
+ def redirect_bug(self):
+ """If +bug traversal fails, redirect to +bugs."""
+ return '+bugs'
+
class BugTaskNavigation(Navigation):
"""Navigation for the `IBugTask`."""
@@ -407,7 +410,9 @@
return None
return getUtility(IBugNominationSet).get(nomination_id)
- redirection('references', '..')
+ @redirection('references')
+ def redirect_references(self):
+ return '..'
class BugTaskContextMenu(BugContextMenu):
=== modified file 'lib/lp/registry/browser/distributionsourcepackage.py'
--- lib/lp/registry/browser/distributionsourcepackage.py 2015-07-09 20:06:17 +0000
+++ lib/lp/registry/browser/distributionsourcepackage.py 2018-07-14 21:01:50 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
__metaclass__ = type
@@ -184,7 +184,9 @@
usedfor = IDistributionSourcePackage
- redirection("+editbugcontact", "+subscribe")
+ @redirection('+editbugcontact')
+ def redirect_editbugcontact(self):
+ return '+subscribe'
def traverse(self, name):
return self.context.getVersion(name)
=== modified file 'lib/lp/services/webapp/doc/canonical_url.txt'
--- lib/lp/services/webapp/doc/canonical_url.txt 2015-07-21 09:04:01 +0000
+++ lib/lp/services/webapp/doc/canonical_url.txt 2018-07-14 21:01:50 +0000
@@ -54,17 +54,19 @@
... Navigation, redirection, stepto, stepthrough)
>>> from zope.publisher.interfaces.browser import IBrowserPublisher
>>> class CountryNavigation(Navigation):
- ... redirection('+capital', '+towns/London')
+ ... @redirection('+capital')
+ ... def redirect_capital(self):
+ ... return '+towns/London'
...
... @stepthrough('+towns')
- ... def stepthrown_town(self, name):
+ ... def stepthrough_town(self, name):
... if name == 'London':
... return town_instance
... else:
... return None
...
... @stepto('+greenwich')
- ... def stepto_greenwhich(self):
+ ... def stepto_greenwich(self):
... town = Town()
... town.name = 'Greenwich'
... return town
=== modified file 'lib/lp/services/webapp/doc/navigation.txt'
--- lib/lp/services/webapp/doc/navigation.txt 2015-07-08 16:05:11 +0000
+++ lib/lp/services/webapp/doc/navigation.txt 2018-07-14 21:01:50 +0000
@@ -29,7 +29,9 @@
- Say that particular steps result in a redirection to some other name.
Example, for a Product:
- redirection('+bug', '+bugs')
+ @redirection('+bug'):
+ def redirect_bug(self):
+ return '+bugs'
- Say how traversal works for anything not handled by the other rules.
Example:
@@ -399,17 +401,14 @@
== redirection ==
You can register that particular names are to be redirected to other names.
-You do this using the redirection() function in one of three ways:
-
- 1. as a class advisor function:
- redirection(namefrom, nameto)
-
- 2. as a Navigation class method decorator:
+You do this using the redirection() function in one of two ways:
+
+ 1. as a Navigation class method decorator:
@redirection(namefrom)
def redirect_namefrom(self):
return nameto
- 3. returned by some other traversal function:
+ 2. returned by some other traversal function:
@stepto(namefrom)
def traverse_namefrom(self):
return redirection(nameto)
@@ -429,8 +428,13 @@
>>> class ThingSetNavigation4(ThingSetNavigation):
...
- ... redirection('tree', 'trees', status=301)
- ... redirection('toad', 'toads')
+ ... @redirection('tree', status=301)
+ ... def redirect_tree(self):
+ ... return 'trees'
+ ...
+ ... @redirection('toad')
+ ... def redirect_toad(self):
+ ... return 'toads'
...
... def traverse(self, name):
... return redirection('/another/place', status=301)
@@ -548,7 +552,9 @@
... def traverse_diplodocus(self, name):
... return 'diplodocus called %s' % name
...
- ... redirection('topology', 'topologies')
+ ... @redirection('topology')
+ ... def redirect_topology(self):
+ ... return 'topologies'
>>> ubernav = UberEverythingThingNavigation(thingset, request)
@@ -605,7 +611,7 @@
'/another/place'
-== Testing that multiple inheritence involving decorator advisors works ==
+== Testing that multiple inheritance involving decorators works ==
>>> class A:
...
=== modified file 'lib/lp/services/webapp/publisher.py'
--- lib/lp/services/webapp/publisher.py 2018-06-21 18:26:31 +0000
+++ lib/lp/services/webapp/publisher.py 2018-07-14 21:01:50 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Publisher of objects as web pages.
@@ -17,9 +17,9 @@
'canonical_url_iterator',
'nearest',
'Navigation',
+ 'redirection',
'rootObject',
'stepthrough',
- 'redirection',
'stepto',
'RedirectionView',
'RenamedView',
@@ -50,7 +50,6 @@
directlyProvides,
implementer,
)
-from zope.interface.advice import addClassAdvisor
from zope.publisher.defaultview import getDefaultViewName
from zope.publisher.interfaces import NotFound
from zope.publisher.interfaces.browser import (
@@ -101,39 +100,31 @@
RESERVED_NAMESPACE = re.compile('\\+\\+.*\\+\\+')
-class DecoratorAdvisor:
- """Base class for a function decorator that adds class advice.
+class DecoratorAnnotator:
+ """Base class for a function decorator that adds an annotation.
- The advice stores information in a magic attribute in the class's dict.
- The magic attribute's value is a dict, which contains names and functions
- that were set in the function decorators.
+ The annotation is stored in a magic attribute in the function's dict.
+ The magic attribute's value is a list, which contains names that were
+ set in the function decorators.
"""
- magic_class_attribute = None
+ magic_attribute = None
def __init__(self, name):
self.name = name
def __call__(self, fn):
- self.fn = fn
- addClassAdvisor(self.advise)
+ if self.magic_attribute is None:
+ raise AssertionError('You must provide the magic_attribute to use')
+ annotations = getattr(fn, self.magic_attribute, None)
+ if annotations is None:
+ annotations = []
+ setattr(fn, self.magic_attribute, annotations)
+ annotations.append(self.name)
return fn
- def getValueToStore(self):
- return self.fn
-
- def advise(self, cls):
- assert self.magic_class_attribute is not None, (
- 'You must provide the magic_class_attribute to use')
- D = cls.__dict__.get(self.magic_class_attribute)
- if D is None:
- D = {}
- setattr(cls, self.magic_class_attribute, D)
- D[self.name] = self.getValueToStore()
- return cls
-
-
-class stepthrough(DecoratorAdvisor):
+
+class stepthrough(DecoratorAnnotator):
"""Add the decorated method to stepthrough traversals for a class.
A stepthrough method must take single argument that's the path segment for
@@ -150,17 +141,17 @@
See also doc/navigation.txt.
- This uses Zope's class advisor stuff to make sure that the path segment
- passed to `stepthrough` is handled by the decorated method.
+ This sets an attribute on the decorated function, equivalent to:
- That is::
- cls.__stepthrough_traversals__[argument] = decorated
+ if decorated.__stepthrough_traversals__ is None:
+ decorated.__stepthrough_traversals__ = []
+ decorated.__stepthrough_traversals__.append(argument)
"""
- magic_class_attribute = '__stepthrough_traversals__'
-
-
-class stepto(DecoratorAdvisor):
+ magic_attribute = '__stepthrough_traversals__'
+
+
+class stepto(DecoratorAnnotator):
"""Add the decorated method to stepto traversals for a class.
A stepto method must take no arguments and return an object for the URL at
@@ -177,58 +168,42 @@
See also doc/navigation.txt.
- This uses Zope's class advisor stuff to make sure that the path segment
- passed to `stepto` is handled by the decorated method.
+ This sets an attribute on the decorated function, equivalent to:
- That is::
- cls.__stepto_traversals__[argument] = decorated
+ if decorated.__stepto_traversals__ is None:
+ decorated.__stepto_traversals__ = []
+ decorated.__stepto_traversals__.append(argument)
"""
- magic_class_attribute = '__stepto_traversals__'
+ magic_attribute = '__stepto_traversals__'
class redirection:
"""A redirection is used for two related purposes.
- It is a class advisor in its two argument form or as a descriptor.
- It says what name is mapped to where.
+ As a function decorator, it says what name is mapped to where.
- It is an object returned from a traversal method in its one argument
- form. It says that the result of such traversal is a redirect.
+ As an object returned from a traversal method, it says that the result
+ of such traversal is a redirect.
You can use the keyword argument 'status' to change the status code
from the default of 303 (assuming http/1.1).
"""
- def __init__(self, arg1, arg2=None, status=None):
- if arg2 is None:
- self.fromname = None
- self.toname = arg1
- else:
- self.fromname = arg1
- self.toname = lambda self: arg2
- addClassAdvisor(self.advise)
+ def __init__(self, name, status=None):
+ # Note that this is the source of the redirection when used as a
+ # decorator, but the target when returned from a traversal method.
+ self.name = name
self.status = status
def __call__(self, fn):
- # We are being used as a descriptor.
- assert self.fromname is None, (
- "redirection() can not be used as a descriptor in its "
- "two argument form")
-
- self.fromname = self.toname
- self.toname = fn
- addClassAdvisor(self.advise)
-
- return fn
-
- def advise(self, cls):
- redirections = cls.__dict__.get('__redirections__')
+ # We are being used as a function decorator.
+ redirections = getattr(fn, '__redirections__', None)
if redirections is None:
redirections = {}
- setattr(cls, '__redirections__', redirections)
- redirections[self.fromname] = (self.toname, self.status)
- return cls
+ setattr(fn, '__redirections__', redirections)
+ redirections[self.name] = self.status
+ return fn
class DataDownloadView:
@@ -941,19 +916,30 @@
getUtility(IOpenLaunchBag).add(nextobj)
return nextobj
- def _combined_class_info(self, attrname):
- """Walk the class's __mro__ looking for attributes with the given
- name in class dicts. Combine the values of these attributes into
- a single dict. Return it.
+ def _all_methods(self):
+ """Walk the class's __mro__ looking for methods in class dicts.
+
+ This is careful to avoid evaluating descriptors.
"""
- combined_info = {}
# Note that we want to give info from more specific classes priority
# over info from less specific classes. We can do this by walking
- # the __mro__ backwards, and using dict.update(...)
+ # the __mro__ backwards, and by callers considering info from later
+ # methods to override info from earlier methods.
for cls in reversed(type(self).__mro__):
- value = cls.__dict__.get(attrname)
+ for value in cls.__dict__.values():
+ if callable(value):
+ yield value
+
+ def _combined_method_annotations(self, attrname):
+ """Walk through all the methods in the class looking for attributes
+ on those methods with the given name. Combine the values of these
+ attributes into a single dict. Return it.
+ """
+ combined_info = {}
+ for method in self._all_methods():
+ value = getattr(method, attrname, None)
if value is not None:
- combined_info.update(value)
+ combined_info.update({name: method for name in value})
return combined_info
def _handle_next_object(self, nextobj, request, name):
@@ -970,7 +956,7 @@
raise NotFound(self.context, name)
elif isinstance(nextobj, redirection):
return RedirectionView(
- nextobj.toname, request, status=nextobj.status)
+ nextobj.name, request, status=nextobj.status)
else:
return nextobj
@@ -986,19 +972,24 @@
@property
def stepto_traversals(self):
"""Return a dictionary containing all the stepto names defined."""
- return self._combined_class_info('__stepto_traversals__')
+ return self._combined_method_annotations('__stepto_traversals__')
@property
def stepthrough_traversals(self):
"""Return a dictionary containing all the stepthrough names defined.
"""
- return self._combined_class_info('__stepthrough_traversals__')
+ return self._combined_method_annotations('__stepthrough_traversals__')
@property
def redirections(self):
"""Return a dictionary containing all the redirections names defined.
"""
- return self._combined_class_info('__redirections__')
+ redirections = {}
+ for method in self._all_methods():
+ for fromname, status in getattr(
+ method, '__redirections__', {}).items():
+ redirections[fromname] = (method, status)
+ return redirections
def _publishTraverse(self, request, name):
"""Traverse, like zope wants."""
Follow ups
