From b072c9b457d04347f21ed86a15f7da618fd9027d Mon Sep 17 00:00:00 2001 From: Gregory Becker Date: Wed, 5 Dec 2018 20:12:47 -0800 Subject: [PATCH] multimethod: slight refactoring, documentation for code review --- lib/spack/spack/multimethod.py | 74 +++++++++---------- lib/spack/spack/spec.py | 1 - lib/spack/spack/test/multimethod.py | 6 ++ .../multimethod-diamond-parent/package.py | 2 +- .../packages/multimethod/package.py | 21 +++++- 5 files changed, 64 insertions(+), 40 deletions(-) diff --git a/lib/spack/spack/multimethod.py b/lib/spack/spack/multimethod.py index b95c8f23419..f7d299659d7 100644 --- a/lib/spack/spack/multimethod.py +++ b/lib/spack/spack/multimethod.py @@ -95,43 +95,45 @@ def __get__(self, obj, objtype): ) 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) + 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) - 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) + 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 @@ def install(self, prefix): 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 2fc06620d29..5563b94e3f0 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -3528,7 +3528,6 @@ def spec(self, name): 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 35ecce7aab2..909ed6d9c16 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 d3413a36b59..e10e5497af0 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 @@ def diamond_inheritance(self): @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 738e41be414..558d99717fd 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 @@ def diamond_inheritance(self): @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'