multimethod: slight refactoring, documentation for code review

5 files changed, 65 insertions, 41 deletions

diff --git a/lib/spack/spack/multimethod.py b/lib/spack/spack/multimethod.py
index b95c8f2341..f7d299659d 100644
--- a/lib/spack/spack/multimethod.py
+++ b/lib/spack/spack/multimethod.py
@@ -95,43 +95,45 @@ class SpecMultiMethod(object):
         )
         return func
 
+    def _get_method_by_spec(self, spec):
+        """Find the method of this SpecMultiMethod object that satisfies the
+           given spec, if one exists
+        """
+        for condition, method in self.method_list:
+            if spec.satisfies(condition):
+                return method
+        return self.default or None
+
     def __call__(self, package_self, *args, **kwargs):
         """Find the first method with a spec that matches the
            package's spec.  If none is found, call the default
            or if there is none, then raise a NoSuchMethodError.
         """
-        for spec, method in self.method_list:
-            if package_self.spec.satisfies(spec):
-                return method(package_self, *args, **kwargs)
-
-        if self.default:
-            return self.default(package_self, *args, **kwargs)
-
-        else:
-            # Unwrap MRO by hand because super binds to the subclass
-            # and causes infinite recursion for inherited methods
-            for cls in inspect.getmro(package_self.__class__)[1:]:
-                superself = cls.__dict__.get(self.__name__, None)
-                if isinstance(superself, self.__class__):
-                    # Parent class method is a multimethod
-                    # check it locally for methods, conditional or default
-                    # Do not recurse, that will mess up MRO
-                    for spec, method in superself.method_list:
-                        if package_self.spec.satisfies(spec):
-                            return method(package_self, *args, **kwargs)
-                    if superself.default:
-                        return superself.default(package_self, *args, **kwargs)
-                elif superself:
-                    return superself(package_self, *args, **kwargs)
-
-            raise NoSuchMethodError(
-                type(package_self), self.__name__, package_self.spec,
-                [m[0] for m in self.method_list]
-            )
-
-    def __str__(self):
-        return "SpecMultiMethod {\n\tdefault: %s,\n\tspecs: %s\n}" % (
-            self.default, self.method_list)
+        spec_method = self._get_method_by_spec(package_self.spec)
+        if spec_method:
+            return spec_method(package_self, *args, **kwargs)
+        # Unwrap the MRO of `package_self by hand. Note that we can't
+        # use `super()` here, because using `super()` recursively
+        # requires us to know the class of `package_self`, as well as
+        # its superclasses for successive calls. We don't have that
+        # information within `SpecMultiMethod`, because it is not
+        # associated with the package class.
+        for cls in inspect.getmro(package_self.__class__)[1:]:
+            superself = cls.__dict__.get(self.__name__, None)
+            if isinstance(superself, SpecMultiMethod):
+                # Check parent multimethod for method for spec.
+                superself_method = superself._get_method_by_spec(
+                    package_self.spec
+                )
+                if superself_method:
+                    return superself_method(package_self, *args, **kwargs)
+            elif superself:
+                return superself(package_self, *args, **kwargs)
+
+        raise NoSuchMethodError(
+            type(package_self), self.__name__, package_self.spec,
+            [m[0] for m in self.method_list]
+        )
 
 
 class when(object):
@@ -193,13 +195,11 @@ class when(object):
        around this because of the way decorators work.
     """
 
-    def __init__(self, spec):
-        if spec is True:
-            self.spec = Spec()
-        elif spec is not False:
-            self.spec = Spec(spec)
+    def __init__(self, condition):
+        if isinstance(condition, bool):
+            self.spec = Spec() if condition else None
         else:
-            self.spec = None
+            self.spec = Spec(condition)
 
     def __call__(self, method):
         # Get the first definition of the method in the calling scope
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 2fc06620d2..5563b94e3f 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -3528,7 +3528,6 @@ class SpecParser(spack.parse.Parser):
             self.check_identifier(spec_name)
 
         if self._initial is None:
-            # This will init the spec without calling Spec.__init__
             spec = Spec()
         else:
             # this is used by Spec.__init__
diff --git a/lib/spack/spack/test/multimethod.py b/lib/spack/spack/test/multimethod.py
index 35ecce7aab..909ed6d9c1 100644
--- a/lib/spack/spack/test/multimethod.py
+++ b/lib/spack/spack/test/multimethod.py
@@ -158,3 +158,9 @@ def test_multimethod_diamond_inheritance():
 
     pkg = spack.repo.get('multimethod-diamond@4.0')
     assert pkg.diamond_inheritance() == 'subclass'
+
+
+def test_multimethod_boolean(pkg_name):
+    pkg = spack.repo.get(pkg_name)
+    assert pkg.boolean_true_first() == 'True'
+    assert pkg.boolean_false_first() == 'True'
diff --git a/var/spack/repos/builtin.mock/packages/multimethod-diamond-parent/package.py b/var/spack/repos/builtin.mock/packages/multimethod-diamond-parent/package.py
index d3413a36b5..e10e5497af 100644
--- a/var/spack/repos/builtin.mock/packages/multimethod-diamond-parent/package.py
+++ b/var/spack/repos/builtin.mock/packages/multimethod-diamond-parent/package.py
@@ -18,4 +18,4 @@ class MultimethodDiamondParent(MultimethodBase):
 
     @when('@4.0, 2.0')
     def diamond_inheritance(self):
-        return "should never be reached"
+        return "should never be reached by diamond inheritance test"
diff --git a/var/spack/repos/builtin.mock/packages/multimethod/package.py b/var/spack/repos/builtin.mock/packages/multimethod/package.py
index 738e41be41..558d99717f 100644
--- a/var/spack/repos/builtin.mock/packages/multimethod/package.py
+++ b/var/spack/repos/builtin.mock/packages/multimethod/package.py
@@ -148,4 +148,23 @@ class Multimethod(MultimethodBase):
 
     @when('@4.0')
     def diamond_inheritance(self):
-        return "should_not_be_reached"
+        return "should_not_be_reached by diamond inheritance test"
+
+    #
+    # Check that multimethods work with boolean values
+    #
+    @when(True)
+    def boolean_true_first(self):
+        return 'True'
+
+    @when(False)
+    def boolean_true_first(self):
+        return 'False'
+
+    @when(False)
+    def boolean_false_first(self):
+        return 'False'
+
+    @when(True)
+    def boolean_false_first(self):
+        return 'True'