Quoting Alec Leamas (2012-08-14 20:09:20)
Stanislaw and I have had a long discussion on IRC today. We have
agreed
on a new interface to the test (a. k. a. check) object to be a bunch of
properties and a single run() method. Compared to today, it means that
the is_applicable() function is no more part of the interface. Instead,
run() returns 'not_applicable' as needed.
Unfortunately, we have designed two ways to implement this in the python
context: one based on inheritance, and one based on decorators. Noone of
us sees it as a major problem to select any solution, but we must
decide. And needs some input.
First method is based on inheritance.
-------------------------------------------------
CheckBase:
def run(self):
if self.is_applicable():
self.run_if_applicable()
else:
self.set_passed('not_applicable')
JavaCheckBase(CheckBase):
def is_applicable(self):
if self.has_files("*.jar") or self.has_files("*.pom"):
return True
else:
return False
A standard test overrides .run_if_applicable(), which only is called if
is_applicable() is true.
def run_if_applicable(self):
name = self.get_files_by_pattern("/usr/share/javadoc/%s" %
self.spec.name)
....
self.set_passed('pass')
For the record, this was my main "problem" with this approach.
"run_if_applicable" suggests that the function either runs only if
applicable or should only be run if applicable, which are completely
different things. If we are to have something in CheckBase I'd like to
find a better name for it. Few things I've though about:
* _run_check "protected" method. (or just run_check)
* something with "implement" in the name :-)
* do_work (seems weird on second though, but listed for completeness :-))
I'd *love* to get some more input about this. I have a hard time
guessing which would be less confusing and more consistent.
Decorator approach goal was not to have run() in CheckBase depend on any
specific function. Compared to the inheritance approach it handles multiple
condition inheritance more gracefully in my opinion.
Example:
class RubyCheckBase(LangCheckBase):
...
@staticmethod
def if_rubypackage(run_f):
def wrapper(self, *args, **kwargs):
if self.is_gem() or self.is_nongem():
return run_f(self, *args, **kwargs)
else:
self.set_passed('not_applicable')
return wrapper
@staticmethod
def if_gempackage(run_f):
def wrapper(self, *args, **kwargs):
if self.is_gem():
return run_f(self, *args, **kwargs)
else:
self.set_passed('not_applicable')
return wrapper
...
class RubyCheckPlatformSpecificFilePlacement(RubyCheckBase):
def __init__(self, base):
...
@RubyCheckBase.if_rubypackage
def run(self):
...
class GemCheckProperName(RubyCheckBase):
def __init__(self, base):
...
@RubyCheckBase.if_gempackage
def run(self):
...
Doing this with inheritance means you have to redefine run(), because you can't
have multiple is_applicable functions. And then would come the
inconsistency...in some cases it's enough to reimplement is_applicable, but in
other cases not. You can of course create another level of inheritance (as the
original Ruby checks do) to solve this problem
Decorators have a different problem and that is manual tests which still have
is_applicable. Since we got rid of it, it has to instead be changed to
automatic check which returns not_applicable when appropriate. Example:
Original:
class CheckLocale(GenericCheckBase):
def __init__(self, base):
...
self.automatic = False
def is_applicable(self):
return self.has_files('/usr/share/locale/*/LC_MESSAGES/*.mo')
New:
class CheckLocale(GenericCheckBase):
def __init__(self, base):
..,
self.automatic = True
def run(self):
if self.has_files('/usr/share/locale/*/LC_MESSAGES/*.mo'):
self.set_passed('inconclusive')
else:
self.set_passed('not_applicable')
Note that the new code does not use decorator. It's not really mandated, it
just seemed like a nicer way to do it than condition at the beginning of every
run()
So...more opinions on the matter?
--
Stanislav Ochotnicky <sochotnicky(a)redhat.com>
Software Engineer - Base Operating Systems Brno
PGP: 7B087241
Red Hat Inc.
http://cz.redhat.com