[PEAK] IsSubclass test reordering [WAS: Patch to clear RuleSet]

[Alberto Valverde]

> At 10:25 PM 7/15/2008 +0200, Alberto Valverde wrote:
>>Phillip J. Eby wrote:
>> > At 11:05 AM 7/15/2008 -0400, Phillip J. Eby wrote:
>> > (....)
>> > Okay, both RuleSet.clear() and a fixed RuleSet.__iter__ are in SVN and
>> > a snapshot build now, along with a temporary fix for the IsSubclass
>> > issue.
>> >
>> > It seems to me that there is more to the issubclass issue than meets
>> > the eye, in that there are potentially other test combinations that
>> > might really need to be ordered similarly.  I'll write up another post
>> > with some thoughts on that as soon as it's practical.
>>
>>Thanks for the quick "temporary" fix :) I've  just uncommented some
>>tests I had which depended on isclass acting as a guard and they all
>> pass.
>
> It wasn't "isclass" that was broken; it was that issubclass() was
> allowed to go first -- didn't matter what the other criteria were.

I grokked that on your first explanation but expressed myself lazily, I
meant to say: "isclass acting as a guard so issubclass is not called".
Sorry.

> The bigger issue is what types of tests should be allowed to go
> first.  Currently, anything but issubclass() that's a test on a plain
> argument is allowed to run in any order, but I can think of error
> scenarios for everything except "is", especially if you consider that
> one argument's contents might dictate the contents of another.

Eeeek, one argument modifying the contents of another in an expression
that will be evaluated when doing rule dispatch sounds like begging for
trouble anyway. peak.rules should not worry about this use-case at all
IMHO but consumers of the library should make sure this can never happen.

For the record, I often modify the arguments passed to a gf but always
make sure to call the "base" function again recursively so rules are
re-evaluated (instead of chaining with next_method). I've sometimes
thought that it would be nice if peak.rules did this automatically for me
but later realized that if it did do that it would be a nightmare to
follow what is really going on (like if it's not non-trivial already until
you get the hang of it).

BTW, any tips on how to trace which functions are being called under what
rules? I hacked something for RuleDispatch some time ago [1] (the
_make_func_logger) but felt kind of dirty. Is there any way to implement
similar functionality, more elegantly, in peak.rules? I've though of an
around() method that logged every next_method's func_name or something but
then I realized that will be called just once for each call to the "base"
gf, not on every "around", "before", "after" and chained "when"s that
might be executed for a single call to the "base" gf (I'm sure there's a
formal name for "base gf"... but unknown to me, sorry). Please note that
this is not a feature request, I know you're a busy man, but a request for
a small bump in the right direction so I can implement it myself.

> The flip side is to just require tests to always occur in the order
> of appearance; this is more predictable and "safer", but could carry
> a performance cost if the order used in the rules as written
> conflicts with the optimum order from a selectivity point of view.

I'm fine with letting peak.rules optimize then order of the tests for me,
in fact, I'd like it to do it as it does now. Just adding some internal
rules to handle these edge cases like you did is enough I think (since
using issubclass to dispatch on the types of classes, although less common
than the type of instances, is a reasonably frequent use-case IMHO).

Maybe this is crazy but... couldn't peak.rules implement an IsSubclass
check that is smarter than python's so it doesn't blow up on non-classes
but return a safer False like isinstance does instead? This would avoid
the isclass test having to be executed before issubclass and in a selfish
sense make my code-base a little smaller ;)

Alberto