launchpad-reviewers team mailing list archive

[Martin Pool <martinpool@xxxxxxxxx>]

Martin Pool has proposed merging lp:~mbp/launchpad/feature-scope-cleanup into lp:launchpad with lp:~mbp/launchpad/mail-cleanup as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~mbp/launchpad/feature-scope-cleanup/+merge/59460

This does a minor cleanup to the feature flag scopes code.  Previously it was a bit hardcoded to the idea of scopes being keyed only off web requests, but we already have some that apply during cron scripts, I soon want to add some for mail requests, and other things are possible in the future.

Now the only scope controller that has the concept of a .request are those that are actually relevant to it.  That seems cleaner than having it present but often None. 

I removed the global WEBAPP_SCOPE_HANDLERS and SCRIPT_SCOPE_HANDLERS because they're a bit redundant with the classes that know how to build them, and they're not used anywhere else.

I tested with './bin/test -m feature', and make lint is clean.
-- 
https://code.launchpad.net/~mbp/launchpad/feature-scope-cleanup/+merge/59460
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~mbp/launchpad/feature-scope-cleanup into lp:launchpad.

=== modified file 'lib/lp/services/features/scopes.py'
--- lib/lp/services/features/scopes.py	2011-03-29 00:11:57 +0000
+++ lib/lp/services/features/scopes.py	2011-04-29 07:22:29 +0000
@@ -1,7 +1,12 @@
 # Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Connect Feature flags into webapp requests."""
+"""Connect feature flags into scopes where they can be used.
+
+The most common is flags scoped by some attribute of a web request, such as
+the page ID or the server name.  But other types of scope can also match code
+run from cron scripts and potentially also other places.
+"""
 
 __all__ = [
     'HANDLERS',
@@ -35,9 +40,6 @@
     # scope.  Also used on +feature-info.
     pattern = None
 
-    def __init__(self, request):
-        self.request = request
-
     @cachedproperty
     def compiled_pattern(self):
         """The compiled scope matching regex.  A small optimization."""
@@ -58,7 +60,14 @@
         return True
 
 
-class PageScope(BaseScope):
+class BaseWebRequestScope(BaseScope):
+    """Base class for scopes that key off web request attributes."""
+
+    def __init__(self, request):
+        self.request = request
+
+
+class PageScope(BaseWebRequestScope):
     """The current page ID.
 
     Pageid scopes are written as 'pageid:' + the pageid to match.  Pageids
@@ -153,7 +162,6 @@
     pattern = r'script:'
 
     def __init__(self, script_name):
-        super(ScriptScope, self).__init__(None)
         self.script_scope = self.pattern + script_name
 
     def lookup(self, scope_name):
@@ -161,25 +169,21 @@
         return scope_name == self.script_scope
 
 
-# Handlers for the scopes that may occur in the webapp.
-WEBAPP_SCOPE_HANDLERS = [DefaultScope, PageScope, TeamScope, ServerScope]
-
-
-# Handlers for the scopes that may occur in scripts.
-SCRIPT_SCOPE_HANDLERS = [DefaultScope, ScriptScope]
-
-
-# These are the handlers for all of the allowable scopes.  Any new scope will
+# These are the handlers for all of the allowable scopes, listed here so that
+# we can for example show all of them in an admin page.  Any new scope will
 # need a scope handler and that scope handler has to be added to this list.
 # See BaseScope for hints as to what a scope handler should look like.
-HANDLERS = set(WEBAPP_SCOPE_HANDLERS + SCRIPT_SCOPE_HANDLERS)
+HANDLERS = set([DefaultScope, PageScope, TeamScope, ServerScope, ScriptScope])
 
 
 class MultiScopeHandler():
-    """A scope handler that combines multiple `BaseScope`s."""
-
-    def __init__(self, request, scopes):
-        self.request = request
+    """A scope handler that combines multiple `BaseScope`s.
+
+    The ordering in which they're added is arbitrary, because precedence is
+    determined by the ordering of rules.
+    """
+
+    def __init__(self, scopes):
         self.handlers = scopes
 
     def _findMatchingHandlers(self, scope_name):
@@ -190,11 +194,11 @@
                 if handler.compiled_pattern.match(scope_name)]
 
     def lookup(self, scope_name):
-        """Determine if scope_name applies to this request.
+        """Determine if scope_name applies.
 
         This method iterates over the configured scope hanlders until it
-        either finds one that claims the requested scope name is a match for
-        the current request or the handlers are exhuasted, in which case the
+        either finds one that claims the requested scope name matches,
+        or the handlers are exhuasted, in which case the
         scope name is not a match.
         """
         matching_handlers = self._findMatchingHandlers(scope_name)
@@ -213,13 +217,17 @@
     """Identify feature scopes based on request state."""
 
     def __init__(self, request):
-        super(ScopesFromRequest, self).__init__(
-            request, [f(request) for f in WEBAPP_SCOPE_HANDLERS])
+        super(ScopesFromRequest, self).__init__([
+            DefaultScope(),
+            PageScope(request),
+            TeamScope(),
+            ServerScope()])
 
 
 class ScopesForScript(MultiScopeHandler):
     """Identify feature scopes for a given script."""
 
     def __init__(self, script_name):
-        super(ScopesForScript, self).__init__(
-            None, [DefaultScope(None), ScriptScope(script_name)])
+        super(ScopesForScript, self).__init__([
+            DefaultScope(),
+            ScriptScope(script_name)])

=== modified file 'lib/lp/services/features/tests/test_scopes.py'
--- lib/lp/services/features/tests/test_scopes.py	2011-03-29 00:11:57 +0000
+++ lib/lp/services/features/tests/test_scopes.py	2011-04-29 07:22:29 +0000
@@ -9,13 +9,14 @@
 from lp.testing import TestCaseWithFactory
 from lp.services.features.scopes import (
     BaseScope,
+    BaseWebRequestScope,
     MultiScopeHandler,
     ScopesForScript,
     ScriptScope,
     )
 
 
-class FakeScope(BaseScope):
+class FakeScope(BaseWebRequestScope):
     pattern = r'fake:'
 
     def lookup(self, scope_name):
@@ -38,17 +39,18 @@
 
     def test_MultiScopeHandler_lookup_ignores_unmatched_scope(self):
         scope_name = self.factory.getUniqueString()
-        handler = MultiScopeHandler(None, [FakeScope(scope_name)])
+        fake_scope = FakeScope(scope_name)
+        handler = MultiScopeHandler([fake_scope])
         self.assertFalse(handler.lookup("other:other"))
 
     def test_MultiScopeHandler_lookup_ignores_inapplicable_scope(self):
         scope_name = self.factory.getUniqueString()
-        handler = MultiScopeHandler(None, [FakeScope(scope_name)])
+        handler = MultiScopeHandler([FakeScope(scope_name)])
         self.assertFalse(handler.lookup("fake:other"))
 
     def test_MultiScopeHandler_lookup_finds_matching_scope(self):
         scope_name = self.factory.getUniqueString()
-        handler = MultiScopeHandler(None, [FakeScope(scope_name)])
+        handler = MultiScopeHandler([FakeScope(scope_name)])
         self.assertTrue(handler.lookup("fake:" + scope_name))
 
     def test_ScopesForScript_includes_default_scope(self):