1:09 p.m.
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')
A test which needs to specify if it's applicable or not can do so by
overriding is_applicable as
today or, more straight-forward, by overriding run()
def run(self):
if not self.has_files("*.pom"):
self.set_passed('not_applicable')
return
...
self.set_passed( "pass")
The other way is based on decorators:
-----------------------------------------------------
CheckBase:
def run(self):
self.set_passed('inconclusive')
JavaCheckBase(CheckBase):
@staticmethod
def if_javapackage(run_f):
def wrapper(self, *args, **kwargs):
if self.has_files("*.jar") or self.has_files("*.pom"):
return run_f(self, *args, **kwargs)
else:
self.set_passed('not_applicable')
return wrapper
A standard test uses a decorator to run as applicable:
@JavaCheckBase.if_javapackage
def run(self):
files = self.get_files_by_pattern("/usr/share/javadoc/%s/*.html" %
self.spec.name)
....
self.set_passed(...)
but can instead choose to handle applicability itself:
def run(self):
if not self.spec.find(self.skip_regex):
self.set_passed('not_applicable')
return
......
self.set_passed(....)
------------------------------------------------------------------
Both methods have their pros and cons. We really need some input to
select a way of doing this. Anyone, out there?
On behalf of Stan
--alec
1:24 a.m.
On Tue, Aug 14, 2012 at 8:09 PM, Alec Leamas <leamas.alec(a)gmail.com> wrote:
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.
I like the inheritance based one best, much easier to read and understand :)
Tim
3:12 a.m.
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
7:22 a.m.
On Wed, Aug 15, 2012 at 10:12 AM, Stanislav Ochotnicky <
sochotnicky(a)redhat.com> wrote:
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')
Merging is_applicable into run and lose the is_applicable method, looks
fine to me, the original idea, what to test if a check was applicable and
if it was, then run the check.
but integrating the check into run() would be good alternative too.
The decorator way is much harder to understand if you are not python
decorator ninja master :)
Tim
2:40 a.m.
On Wed, 2012-08-15 at 14:22 +0200, tim.lauridsen(a)gmail.com wrote:
On Wed, Aug 15, 2012 at 10:12 AM, Stanislav Ochotnicky <
sochotnicky(a)redhat.com> wrote:
> 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')
>
Merging is_applicable into run and lose the is_applicable method, looks
fine to me, the original idea, what to test if a check was applicable and
if it was, then run the check.
but integrating the check into run() would be good alternative too.
The decorator way is much harder to understand if you are not python
decorator ninja master :)
Catching up on this subject I was getting the same idea. Since the tests
can override the run() method anyway, let's just have them do that and
return 'not_applicable' when it is.
Let's have each test decide how they want to handle the is_applicable().
Imho, we should go for KISS :-)
Pierre
4 a.m.
On 08/16/2012 09:40 AM, Pierre-Yves Chibon wrote:
On Wed, 2012-08-15 at 14:22 +0200, tim.lauridsen(a)gmail.com wrote:
> On Wed, Aug 15, 2012 at 10:12 AM, Stanislav Ochotnicky <
> sochotnicky(a)redhat.com> wrote:
>
>> 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')
>>
>
> Merging is_applicable into run and lose the is_applicable method, looks
> fine to me, the original idea, what to test if a check was applicable and
> if it was, then run the check.
> but integrating the check into run() would be good alternative too.
> The decorator way is much harder to understand if you are not python
> decorator ninja master :)
Catching up on this subject I was getting the same idea. Since the tests
can override the run() method anyway, let's just have them do that and
return 'not_applicable' when it is.
Let's have each test decide how they want to handle the is_applicable().
Imho, we should go for KISS :-)
Pierre
Seems that many people tend to think using inheritance is the most
straight-forward way. I agree on that.
I also hope and think we agree on that the default behaviour should be
defined so that manual tests (the vast majority) normally shouldn't
need to handle applicability or define run(). This also means that the
vast majority of the automatic tests could rely on the default
applicablility. I think this makes it easier both to write and read
tests (making special treatment visible).
Seems that Stan just don't like the run_if_applicable() way of
overriding. Maybe just renaming that function could make life easier?
run_when_applicable(), applicable_run(), whatever...) But keeping the
external run() interface means that the function called after checking
is_applicable needs another name...
There's another aspect: looking into Mikolaj's great proposal of a
simplifed shell interface it becomes obvious that to create simple tests
they should only declare which group they belong to (.e. g. 'Java' or
'Generic') and then only be invoked if applicable. Something like
#!/bin/bash
#@group: Java
test "something"
exit $?
But that means that the plugin handling these tests needs to use a
'is_applicable' function for every group. This is *not* part of the
Check interface, it's part of the module interface: A test module
handles a specific group, register a list of tests for it and has a
default 'is_applicable() function. Given this, it seems logical that
tests by default respect this is_applicable() function.
So, in a way, the interface function is_applicable() is resurrected. In
the check-api parlance it means that is_applicable() is part of the
AbstractRegistry interface, but not AbstractCheck.
--alec
4270
days inactive
4272
days old
fedorareview@lists.fedorahosted.org
5 comments
5 participants
participants (5)
-
Alec Leamas -
Pierre-Yves Chibon -
Stanislav Ochotnicky -
Tim Lauridsen -
Tim Lauridsen