callbacks icon indicating copy to clipboard operation
callbacks copied to clipboard

Manual callbacks

Open chadrik opened this issue 9 years ago • 8 comments

This is a pretty big refactor, and there's a chance you won't be interested in merging it, but I thought I'd give it a try. I've reorganized the internals into smaller classes, so that custom recipes can be created from them. My use case is that I want something very like what callbacks provides, but I need my functions to be in control of when the event fires (i.e. it is neither before or after the target function, but somewhere in the middle).

  • callbacks are organized by event: An Event class is used to register callbacks. it knows nothing about the target.
  • an event is triggered by calling Event.emit(), and may be sub-classed to add special behavior
  • a CallbackRegistry stores the target and one or more events as attributes. Callbacks are registered like myfunction.event_name.add_callback
  • The existing automatic functionality is implemented using these various parts on AutoCallbacks. The add_*_callback methods are available on this class, but the new style for adding callbacks is via the events: e.g. myfunction.on_call.add_callback, myfunction.on_return.add_callback, and myfunction.on_exception.add_callback
  • ids are now generated based on the callback function rather than uuids, so that callbacks can be removed without storing the returned id
  • added python3 support

This is not done yet. I still need to update docs, examples, and tests for the manual event emission, but I wanted to see what you thought first. I've made a few breaking changes, but I can remove a most of them with a little more work.

chadrik avatar Nov 03 '16 06:11 chadrik

Coverage Status

Coverage decreased (-4.8%) to 94.561% when pulling 575fc7d1b9455ca0ae79b40ab242f0c3f63f6a51 on chadrik:manual_callbacks into 124cfc9cf905d4f58c4336e03c223445e784a12e on davidlmorton:master.

coveralls avatar Nov 03 '16 06:11 coveralls

Coverage Status

Coverage decreased (-4.8%) to 94.561% when pulling 575fc7d1b9455ca0ae79b40ab242f0c3f63f6a51 on chadrik:manual_callbacks into 124cfc9cf905d4f58c4336e03c223445e784a12e on davidlmorton:master.

coveralls avatar Nov 03 '16 06:11 coveralls

My only concern would be that you've now disallowed adding the same function as a callback more than once. Although it probably isn't a very useful thing to want to do. :) I'd be happy to merge your PR once you've updated docs and examples. Glad you're so interested in the project.

davidlmorton avatar Nov 04 '16 02:11 davidlmorton

Coverage Status

Coverage decreased (-42.1%) to 57.306% when pulling ac1738cfb29935f03aa1dd83f86499ab719d5841 on chadrik:manual_callbacks into 05afa60b1160670682afb3e0c9f6941771078764 on davidlmorton:master.

coveralls avatar Nov 04 '16 16:11 coveralls

Coverage Status

Coverage decreased (-7.5%) to 91.941% when pulling 243543a675206844960aca8a02bd5a02c2614cba on chadrik:manual_callbacks into 05afa60b1160670682afb3e0c9f6941771078764 on davidlmorton:master.

coveralls avatar Nov 04 '16 16:11 coveralls

Coverage Status

Coverage decreased (-3.8%) to 95.636% when pulling 0e23602a860bc6aac2d0de874311107299a19d2b on chadrik:manual_callbacks into 05afa60b1160670682afb3e0c9f6941771078764 on davidlmorton:master.

coveralls avatar Nov 04 '16 17:11 coveralls

@davidlmorton glad to hear you're open to merging it. If you have any feedback on the design of the new event stuff, I'd be glad to hear it.

I'm currently reusing @supports_callbacks to register manual events, but I've debated back and forth over whether it's more or less confusing to introduce a separate decorator just for that purpose, such as @event_emitter, @supports_event_callbacks, etc. Here's how you can register custom/manual events:

from callbacks import supports_callbacks, ReturnEvent

def iter_callback(i):
    print i

def result_callback(result):
    print result 

@supports_callbacks('on_iteration', pass_args=True)
@supports_callbacks('on_return', ReturnEvent, pass_result=True)
def pair_range(num):
    result = []
    for i in range(num):
        pair_range.on_iteration.emit(i)
        result.append((i, i + 1))
    pair_range.on_return.emit(target_result=result)
    return result

pair_range.on_iteration.add_callback(iter_callback)
pair_range.on_return.add_callback(result_callback)
pair_range()

For triggering events do you prefer emit() or fire(), or something else?

Also, I've left add_pre_callback, add_post_callback, and add_exception_callback for backwards compatibility, but the "real" methods are on the events: on_call.add_callback, on_return.add_callback, and on_exception.add_callback. Which would you prefer that the examples use?

chadrik avatar Nov 04 '16 18:11 chadrik

I have a few comments/suggestions and a proposal. First, the suggestions:

  • The emit method could just be implemented in __call__ (then we don't have to figure out a name).
  • I noticed the docstrings on things decorated with supports_callbacks are showing %s instead of function or method. I do think the docstrings are a nice-to-have feature, but they aren't critical in my opinion. You may want to look at the decorators package either for inspiration or just to use it so that things decorated with supports_callbacks preserve all the introspection of the target function/method.
  • I'm not a big fan of the syntax choice involving multiple decorations. I'd prefer to just decorate once with a list of Events. If you want to allow more customization (per Event) you could accept either an Event class or an instance of an Event class. The Event's settings could be class-variables which are overridable by instance variables in __init__. I hope all that made some kind of sense. Here is a silly example:
class CustomGuy(Event):
    attribute_name = "on_custom_guy"
    some_setting = True
    def __init__(self, attribute_name="on_custom_guy", some_setting=True):
        self.attribute_name = attribute_name
        self.some_setting = some_setting

c = CustomGuy(attribute_name="on_altered_custom_guy", some_setting=False)

@supports_callbacks([c, CustomGuy])
def foo():
    pass

def bar():
    pass

foo.on_custom_guy.add_callback(bar)
foo.on_altered_custom_guy.add_callback(bar)
  • I'd consider renaming Event to CallbackRegistry and find a better name for the existing CallbackRegistry. It really does appear to be a registry more than an event. It does "happen" (or fire) at some point in time too though, so naming it is pretty hard.

And now for the proposal. I don't have a ton of interest in continuing to own callbacks. As you appear to be both highly capable and interested in it, I'd be happy to transfer the project to your stewardship. If that interests you email me at davidlmorton at gmail and we can coordinate (for example I could transfer the pypi project to you as well).

davidlmorton avatar Nov 05 '16 06:11 davidlmorton