milibopp / obsub

Small python module that implements the observer pattern via a decorator.
Other
12 stars 3 forks source link

Black magic #46

Closed coldfix closed 6 years ago

coldfix commented 10 years ago

I finally managed to create a reasonable version that uses black_magic.decorator to create nice function objects that preserve parameter names and default arguments even on python2.

Possible issues to take into consideration:

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling 7f3a13d46287d008f16fdebae0b7a76e6e3e926c on coldfix:black_magic into acf8d4fe908ec3aa665324cbaa868add39166dd2 on aepsil0n:master.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling 73dd4d6f336aee271b20586b9ec65bce524ed18f on coldfix:black_magic into acf8d4fe908ec3aa665324cbaa868add39166dd2 on aepsil0n:master.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling 3c32e60c724db16d21473cc951cb4b186b321539 on coldfix:black_magic into acf8d4fe908ec3aa665324cbaa868add39166dd2 on aepsil0n:master.

milibopp commented 10 years ago

Great work! This is a major rewrite, so it'll take some time to review.

Regarding the black_magic dependency: it does preserve function signatures, so this is probably important functionality beyond documentation, is it not? Otherwise, could you reasonably strip it out of this PR? I'm not saying that you should, just trying to assess our options here.

coldfix commented 10 years ago

It is possible to exchange black_magic.decorator.wraps for functools.wraps. This makes the returned handlers less nice for help() and drops support for default parameters. This should allow obsub to be used as a drop-in more easily. I have provided a commit that implements this. Coverage will probably go down a bit, since the ImportError control path isn't executed.

coldfix commented 10 years ago

Btw., c5b00ff (_Use function.get instead of blackmagic.partial) should restore approximately the original speed but makes it much harder to decorate anything but basic functions. So, if we care about covering many use cases, we should probably revert this.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-30.19%) when pulling a7a7d24a140b04e543bd3be97d9a48c8cce8691b on coldfix:black_magic into acf8d4fe908ec3aa665324cbaa868add39166dd2 on aepsil0n:master.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling d2af23df2db447fa93d955ecbc9c83d9577d8a57 on coldfix:black_magic into 29c47141460f788f6eb7719d0309abdf383871e2 on aepsil0n:master.

coldfix commented 10 years ago

Finally, I managed to do some rudimentary speed tests.. Turns out, that black_magic.wraps indeed has a pretty strong performance hit.

The code in the latest change may seem a little confusing, but it mostly fixes the performance issue by moving the only wraps call to the contructor. Furthermore, the events are now practically indistinguishable from normal member functions. Also, it should be possible again to decorate some non-function objects.

If you are interested in the benchmark:

from obsub import event
import timeit

def create():
    class X(object):
        @event
        def foo(self, bar=3):
            return bar
    return X

X = create()
x = X()

def invoke():
    x.foo()
    x.foo(2)

def access():
    X.foo

def bench(func, number=10000):
    return timeit.timeit("%s()" % func,
                         setup="from __main__ import %s" % func,
                         number=number)

if __name__ == '__main__':
    print(bench('create'))
    print(bench('invoke'))
    print(bench('access'))

Result on CPython2.7

master a7a7d24 d2af23d
create 0.665 0.661 6.092
invoke 0.253 9.753 0.105
access 0.006 4.574 0.005

This looks similar onCPython2.6 and CPython3.3.

On pypy, however, the optimization doesn't make much difference:

master a7a7d24 d2af23d
create 0.264 0.266 0.3
invoke 0.164 0.164 0.083
access 0.063 0.063 0.072
coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling 9baec0c7359da7eb9f03e53b46e914a64de16ced on coldfix:black_magic into 29c47141460f788f6eb7719d0309abdf383871e2 on aepsil0n:master.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling bfc233925135a03d90e0f897f2e5683f18b0d775 on coldfix:black_magic into 29c47141460f788f6eb7719d0309abdf383871e2 on aepsil0n:master.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling bfc233925135a03d90e0f897f2e5683f18b0d775 on coldfix:black_magic into 29c47141460f788f6eb7719d0309abdf383871e2 on aepsil0n:master.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-16.25%) when pulling e707033ea982d97fdaaa911f82e4018a6c4fea19 on coldfix:black_magic into 29c47141460f788f6eb7719d0309abdf383871e2 on aepsil0n:master.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-19.12%) when pulling 78be84cbb3d0536742410e3f27e6c8961ec96844 on coldfix:black_magic into 29c47141460f788f6eb7719d0309abdf383871e2 on aepsil0n:master.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-19.12%) when pulling ad7329d0ba375ab265c3070517f603793f20b7ff on coldfix:black_magic into 29c47141460f788f6eb7719d0309abdf383871e2 on aepsil0n:master.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-14.44%) when pulling a10516d4bf027f9110415a279e26df2de169e306 on coldfix:black_magic into 29c47141460f788f6eb7719d0309abdf383871e2 on aepsil0n:master.

coldfix commented 10 years ago

There is a fatal flaw in this branch: Assuming a.foo is an event, a.foo == a.foo will always evaluate to false. This will break code like x.connect(a.foo) followed by x.disconnect(a.foo).

Reason: event.__get__ returns a new copy of a function each time and function comparison is done by identity. Furthermore, in python it is not possible to overload equality of functions (it is not possible to derive from FunctionType/MethodType).

The only possibility I can see to get the desired behaviour, is to revert back to a bound_event class. This in turn, will make automatic help() behave much less nicely, and therefore, the only reason left for the black_magic dependency is the support for default arguments in python2.