IDLE - patch Delegator to support callables

[taleinat]

BPO	1252
Nosy	@kbkaiser, @taleinat
Files	IDLE_Delegator.071010.patch delegator.txt delegator2.txt Delegators3.py example1.py example1a.py example2.py

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/kbkaiser'
closed_at = <Date 2008-01-14.00:33:31.770>
created_at = <Date 2007-10-10.02:06:50.776>
labels = ['expert-IDLE', 'type-bug']
title = 'IDLE - patch Delegator to support callables'
updated_at = <Date 2010-07-21.09:32:46.234>
user = 'https://github.com/taleinat'

bugs.python.org fields:

activity = <Date 2010-07-21.09:32:46.234>
actor = 'taleinat'
assignee = 'kbk'
closed = True
closed_date = <Date 2008-01-14.00:33:31.770>
closer = 'kbk'
components = ['IDLE']
creation = <Date 2007-10-10.02:06:50.776>
creator = 'taleinat'
dependencies = []
files = ['8501', '8604', '8635', '8637', '8654', '8668', '8669']
hgrepos = []
issue_num = 1252
keywords = ['patch']
message_count = 11.0
messages = ['56305', '56723', '56751', '56862', '56869', '56874', '56913', '56984', '56987', '58395', '111029']
nosy_count = 2.0
nosy_names = ['kbk', 'taleinat']
pr_nums = []
priority = 'normal'
resolution = 'rejected'
stage = None
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue1252'
versions = ['Python 2.6', 'Python 2.5']

[taleinat]

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.

[kbkaiser]

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?

[taleinat]

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.

[kbkaiser]

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?

[taleinat]

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?

[kbkaiser]

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).

[taleinat]

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>

[kbkaiser]

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.

[kbkaiser]

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.

[kbkaiser]

Do you have any further comments on this issue?

[taleinat]

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.