Created on 2007-10-10 02:06 by taleinat, last changed 2010-07-21 09:32 by taleinat. This issue is now closed.
|IDLE_Delegator.071010.patch||taleinat, 2007-10-10 02:06|
|delegator.txt||kbk, 2007-10-24 19:34|
|delegator2.txt||kbk, 2007-10-27 21:34|
|Delegators3.py||taleinat, 2007-10-28 00:28|
|example1.py||taleinat, 2007-10-29 17:33|
|example1a.py||kbk, 2007-10-31 05:21|
|example2.py||kbk, 2007-10-31 05:21|
|msg56305 - (view)||Author: Tal Einat (taleinat)||Date: 2007-10-10 02:06|
Add an __call__ magic method to Delegator so that it can delegate to callables. This will raise the required exception if the delegate isn't callable. The built-in callable() method will now return True for any Delegator. Before this patch it always returns False, which is wrong if the delegate is callable, so callable() doesn't really work on Delegator instances in both cases. I couldn't see how this could be harmful. Also make the Delegator class a new-style class. This patch is required for the Squeezer and ShellLogger IDLE extensions found on PyPI to be able to co-exist.
|msg56723 - (view)||Author: Kurt B. Kaiser (kbk) *||Date: 2007-10-24 19:34|
Despite your explanation, I don't understand what is being accomplished here. Delegates are not intended to be callable. They have methods, e.g. insert, which are callable, and the insert call is propagated down the chain by calls like (from ColorDelegator): def insert(self, index, chars, tags=None): index = self.index(index) self.delegate.insert(index, chars, tags) self.notify_range(index, index + "+%dc" % len(chars)) IMHO it's an incorrect usage of the Delegator mixin to affect the callable nature of the class to which it's being added. Also, this __call__ method, if actually used, propagates to the end of the Delegator chain and calls the function at the end, (assuming it is a function). Or it will skip non-callable Delegators and stop at the first callable one. I doubt this is what was intended, and people trying to understand the code will be further confused, it seems to me. Try adding delegator.txt (below) to your Delegator.py and running the module! I think this patch increases the complexity and obscurity of the Delegator/Percolator/WidgetRedirector code, rather than improving it. Apparently you want to sometimes call a chain of methods (as in current usage) and sometimes call a chain of functions. The semantic overload, which was bad enough already, is too great. If Squeezer and ShellLogger require this change, then I'd suggest changing them to match current Percolator usage, instead. Currently I don't see a Delegator being used in Squeezer. Could you be more specific about why the change is needed for those two extensions?
|msg56751 - (view)||Author: Tal Einat (taleinat)||Date: 2007-10-25 19:32|
I understand your argument, but am not convinced. I'll try to explain how I see this and how I came to this view. I wished to wrap a specific method (or function) instead of wrapping the father object with a Delegator and implementing a single method. But I needed more than simple function wrapping - I wanted a chain of functions where each wraps the next, and the ability to change the order of links in this chain. So what I really wanted was a Percolator, but instead of having it wrap an object and delegate methods, I wanted it to delegate direct calls. My first idea was to implement a FunctionDelegator class. But I realized I would have to implement a FunctionPercolator class, which would be indentical to the existing Percolator class except that it would inherit from FunctionDelegator instead of Delegator. From there I had three choices: 1) create a second pair of delegator/percolator classes which are nearly identical (much code ducplication) 2) make a Percolator generator function, which accepts a Delegator class, and returns a Percolator class/object which uses it (possibly inheriting from it) 3) make Delegator able to delegate direct calls The third option is obviously the simplest to implement, requiring the least code without any code duplication. After some thought, I came to the point-of-view described below, and realized that the third option is actually what I would expect if I were thinking about delegators and percolators as transparent proxies. > Delegates are not intended to be callable. Why not? IMO, the nice thing about the Delegator class is that you can use an instance just as if it were the underlying object, transparently. The major exception from this behavior was that calling a Delegator never works, even if the underlying object is callable. From this point of view, my patch makes the Delegator concept complete, while before it was broken. > I think this patch increases the complexity and obscurity of the > Delegator/Percolator/WidgetRedirector code, rather than improving > it. Apparently you want to sometimes call a chain of methods (as > in current usage) and sometimes call a chain of functions. The > semantic overload, which was bad enough already, is too great. You describe method calls and direct calls as though they were fundamentally different. Therefore you find that delegating both is too much semantic overloading. However, if one views direct calling of an object as a special case of calling a method (since functions are also objects) -- the __call__ method -- then delegating both method calls and direct calls is The Right Thing. > Also, this __call__ method, if actually used, propagates to the > end of the Delegator chain and calls the function at the end, > (assuming it is a function). Or it will skip non-callable > Delegators and stop at the first callable one. True, since this is precisely what Delegators do for any method call -- this is the expected behavior. > If Squeezer and ShellLogger require this change, then I'd > suggest changing them to match current Percolator usage, instead. > Currently I don't see a Delegator being used in Squeezer. Could > you be more specific about why the change is needed for those > two extensions? Squeezer currently replaces PyShell's write method with a method of its own. ShellLogger also needs to hook onto the write method, but ShellLogger's function must be called before Squeezer's method, since Squeezer can insert a button without calling the underlying write method. But extensions must replace the write method in their constructor, and the order of their construction is "random". In other words, a rudimentary form of constraint enforcement is required, which can be achieved using a Percolator. Without the suggested change to Delegator, this can be accomplished by wrapping EditorWindow with a Percolator, and having extensions insert Delegators (filters) as appropriate. However, this is a much larger change to the code -- for example, this breaks type checks, e.g. assert isinstance(editwin, EditorWindow)). This would also add overhead to every method call done on EditorWindow, with this overhead growing linearly with the number of Delegators inserted. I find wrapping specific methods to be more straightforward and therefore simpler, but this can be argued.
|msg56862 - (view)||Author: Kurt B. Kaiser (kbk) *||Date: 2007-10-27 21:34|
First, I'm changing my mind about Percolator inheriting from Delegator. A Percolator acts as a container for Delegators: it "hasa" (chain) of them. But it fails "isa" Delegator. It has no use for the Delegator's caching, and chaining Percolators doesn't make sense. Inheriting from Delegator just confuses things further, it seems to me. Delegator is just a mixin implementing a node in the chain. I do support splitting TkTextPercolator off Percolator. > 3) make Delegator able to delegate direct calls Except that the Delegator mixin doesn't know to what function to delegate the call. Delegating a function call down the nodes doesn't do anything, except possibly error out if the bottom object isn't callable, as in delegator.txt. > IMO, the nice thing about the Delegator class is that you can > use an instance just as if it were the underlying object, transparently. > The major exception from this behavior was that calling a Delegator > never works, even if the underlying object is callable. But it does work, if the filter that is using the Delegator mixin has a __call__ method. See delegator2.txt above. Note that the Delegator __call__ method is removed. You have to override the latter anyway if you want to run some code in the filter. Do you have some reason for mixing callable and non-callable filter instances in the percolator chain? I can see adding a __call__ method to Percolator, which would call self.top(). Then each instance in the chain would have a __call__ to appropriate code. We have two goals: solve your specific requirement of being able to replace a method with a percolator, and increasing the clarity of the existing WidgetRedirector/Delegator/Percolator code. Yes, a Percolator already has semantic overload. Right now, there are two ways to access the chain: 1. Since the delegate link is exposed, filters can directly call specific methods further down the chain, e.g. self.delegate.index() 2. The caching and __getattr__() allows a "delegator" to call unimplemented methods; they will be looked up on the chain. This allows ColorDelegator to access its Text instance's methods without having been passed a reference to the instance, as you noted. Whether this adds or detracts from the clarity of the code is debatable. Once you understand how it works, it's not a problem, but it would be for people new to the code. Further, it's fragile, since the first method with the correct name will be called. Adding a __call__() method to Delegator doesn't seem to do anything that can't be accomplished better by adding it to the class implementing the filter. Why add complexity prematurely?
|msg56869 - (view)||Author: Tal Einat (taleinat)||Date: 2007-10-28 00:28|
It seems we're looking at Delegators and Percolators from increasingly different points of view. Let's back up a little. I see a Delegator object as a transparent proxy to its "delegate". This means that attribute access is automatically delegated to the delegate, unless it is explicitly overridden by the Delegator. That's it. Some use cases for such a transparent proxy are: * override only specific attributes/methods of an object * allow replacement of an object which is referenced in several places without having to update every reference (Caching is just an implementation detail, whose only purpose is to facilitate changing a Delegator's delegate.) As for Percolator, it really "is a" delegator -- it delegates attribute access to the bottom of the chain, unless it is explicitly overridden. True, in a "normal" Delegator this overriding can only be done in one place, and in a Percolator it can also happen in any of the chain's links. But the concept is identical -- it is a transparent proxy for an underlying object. IMO chaining Percolators makes just as much sense as chaining Delegators -- you're just chaining proxies. How each proxy works internally doesn't really matter (as long as they work :). Now, it seems to me that you aren't looking at Delegators and Peroclators as transparent proxies at all. Specifically, what you wrote implies that in order to proxy a callable, one should explicitly define an __call__ method in their Delegator class/instance. But this is exactly the opposite of the behavior with any other method/attribute, where I can implicitly have the underlying attribute used by not defining it in the Delegator. This is Delegator is for! I'm attaching a Python file which will hopefully show how __call__ is out of sync with the rest of Delegator's behavior. In its context, "forwarded" means explicitly defined by a Delegator. "intercepted" means that except for the interceptor and catcher, the method is not defined (i.e. by the passers). Please take a moment to run it. I should note that the situation is similar with other "magic" methods, e.g. len(). This seems to make Python a bit less dynamic that I would expect. Aside from implementation considerations such as speed, I'm not sure I see why this is the way it is, e.g. why dynamically giving a __call__ attribute to an instance shouldn't make it callable. I'll do some more searching and reading on this. Even though, I still think being able to delegate/percolate callables is important enough to warrant such a change. After all, at the bottom line, if the underlying object is callable then it will be called, and if not then an appropriate exception will be raised. Isn't that the Right Thing?
|msg56874 - (view)||Author: Kurt B. Kaiser (kbk) *||Date: 2007-10-28 02:42|
I'll respond further shortly. In the meantime, please notice that Delegator3.py works the same whether or not your Delegator.__call__() method is commented out. That's because you needed to define __call__() methods in your filters. We are still suffering from semantic overload. Let's call the instances which are chained 'filters' and the Delegator mixin machinery 'nodes' for the purposes of this discussion (because they act much like the nodes in a traditional Lisp list).
|msg56913 - (view)||Author: Tal Einat (taleinat)||Date: 2007-10-29 17:33|
I'm trying to keep this as simple as possible, because it seems we're tending to over-complicate. We're discussing two distinct issues: 1) Should Delegator delegate calls to callables 2) Should Percolator inherit from Delegator Have I missed something? Regarding the first issue, I do think Delegator should do this, because I view calling a callable as a special case of calling a method. To illustrate my point, please take the code in example1.py, run it once with Delegator as it is, and run it again after adding the __call__ method to Delegator's definition. Regarding the second issue, I don't think I can put my thoughts better than I already have: <quote> As for Percolator, it really "is a" delegator -- it delegates attribute access to the bottom of the chain, unless it is explicitly overridden. True, in a "normal" Delegator this overriding can only be done in one place, and in a Percolator it can also happen in any of the chain's links. But the concept is identical -- it is a transparent proxy for an underlying object. </quote>
|msg56984 - (view)||Author: Kurt B. Kaiser (kbk) *||Date: 2007-10-31 05:00|
Further response to your 27Oct: > That's it. There is more. The Delegator mixin exposes its delegate attribute. Without that, it would not be possible to pass e.g. insert() down the chain because (in the case of the Text percolator) insert() is found in each filter and blocks 'transparent' access. I agree with your two use cases, but repeat that transparent access is dangerous in that the class in which the attribute is being looked up changes for each link on the chain. You could get unexpected results. IMO you are giving up stability for convenience. "Explicit is better than implicit." > (Caching is just an implementation detail, whose only purpose is to > facilitate changing a Delegator's delegate.) Don't believe everything you read. While that comment in the code is true, it's not the whole truth. If 'transparent' access is made to an attribute further down the chain, that attribute will be actually cached, i.e. be set as an attribute, in each DelegatorNode. I imagine this was done for performance reasons. The Delegator.__cache is used to determine which attributes to delete if a delegate is changed. I'll defer the Percolator comments until later. > Now, it seems to me that you aren't looking at Delegators and > Peroclators as transparent proxies at all. Not so. That's my 2. in my msg56862 27Oct. But I see filters as having two modes of operation. In the first, they take an action like insert() and share it explicitly along a chain of authority. Each link takes specific action, and passes insert() along. They also provide transparent access to attributes down the chain, as you note. But once an attribute is found it will not propagate unless explicitly passed along, and that requires the self.delegate attribute.
|msg56987 - (view)||Author: Kurt B. Kaiser (kbk) *||Date: 2007-10-31 05:18|
> 1) Should Delegator delegate calls to callables No, I agree they should. The question is whether it's necessary to add a __call__() method to the Delegator class. I claim you can do what you want to do without it. It serves only one purpose that I can see: you want to delegate to a callable by calling the instance at the top of the chain, and one of the chain members isn't callable. I don't see a use case for that, YAGNI. I'll defer the Percolator discussion for now. There is more to it than just the inheritance issue. I found your example1.py to be confusing. Please stop using the word 'delegator', it's too overloaded in this discussion! I marked up example1.py as example1a.py, changing some names and adding some comments. I also worked up example2.py which seems to cover the possible combinations of proxying functions, callables, and methods. It works without adding __call__() to Delegator. Please check it out, what more do you need? I'll be away until Sunday, so I can't respond further until then.
|msg58395 - (view)||Author: Kurt B. Kaiser (kbk) *||Date: 2007-12-11 00:37|
Do you have any further comments on this issue?
|msg111029 - (view)||Author: Tal Einat (taleinat)||Date: 2010-07-21 09:32|
I'm sorry I let this die out. This should stay closed. For the time being I have given up on developing the ShellLogger extension (for the time being) so this change is not needed as far as I am concerned.
|2010-07-21 09:32:46||taleinat||set||messages: + msg111029|
|2008-01-14 00:33:31||kbk||set||status: open -> closed|
|2007-12-11 00:37:44||kbk||set||messages: + msg58395|
|2007-10-31 05:21:58||kbk||set||files: + example2.py|
|2007-10-31 05:21:29||kbk||set||files: + example1a.py|
|2007-10-31 05:18:16||kbk||set||messages: + msg56987|
|2007-10-31 05:00:51||kbk||set||messages: + msg56984|
messages: + msg56913
|2007-10-28 02:42:48||kbk||set||messages: + msg56874|
messages: + msg56869
|2007-10-27 21:34:53||kbk||set||priority: normal|
messages: + msg56862
files: + delegator2.txt
|2007-10-25 19:32:54||taleinat||set||messages: + msg56751|
messages: + msg56723
|2007-10-10 06:03:02||loewis||set||keywords: + patch|