i2mint / know

Funnel live streams of data into storage and other processes
Apache License 2.0
0 stars 0 forks source link

What do we want `ContextFunc`'s interface and behavior to be? #5

Closed thorwhalen closed 1 year ago

thorwhalen commented 2 years ago

In plunk/ap/live_graph/live_graph_data_buffer.py, @andeaseme fixed issue with tracking is not flushed when SlabsIter exits.

The problem was with ContextualFunc requiring both arguments to be wrapped. The func arg needs to be wrapped to remove self arg. The contexts need to be wrapped in a list if it's a single context. ContextualFunc does too much by handling multiple contexts. ContextFanout can be used to wrap when a multiple context situation arrives.

So what do we want ContextFunc's interface and behavior to be?

andeaseme commented 1 year ago

Changed to ContextFunc(func, contexts, named_contexts) to leverage python's (*args, *kwargs) and using ContextFanout under the hood. No extra list wrapping needed for single context. ContextFunc(func, [context1]) becomes simply ContextFunc(func, context1). Mulit-context ContextFunc(func, [context1, context2]) becomes ContextFunc(func, context1, context2) or unpack with if it is programmatically generated list of context ContextFunc(func, *[context1, context2, ...])

The issue with func needing to be wrapped seems to come from another source so no changes made here to address it.

thorwhalen commented 1 year ago

I too am like to use variadics (*args and **kwargs) when ever I can. For the advantage you mention: It makes for a nicer user experience (especially to express a single positional argument), but it should be noted that the advantage is not that clearcut.

The problem run into when I went the variadic way is if I later want to add some arguments. Now these argument clash with the variadics. For example, if I want to add a verbose flag, I probably need to call it _verbose in case the user needs to have verbose in their **kwargs.

That said, since we already use variadics in i2.multi_obj, it's probably better that ContextualFunc stay consistent with that (especially, that it might end up there anyway).

I'll go ahead and accept your push request.