Opened 14 years ago

Closed 14 years ago

#2901 enhancement closed fixed (fixed)

proxyForInterface should allow upcalls

Reported by: Jonathan Lange Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: radix, Glyph, Jonathan Lange, therve Branch: branches/proxyForInterface-upcall-2901-2
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description

Currently:

class IFoo(Interface):
    def foo():
        pass

class TrivialDecorator(proxyForInterface(IFoo)):
    pass

class LessTrivialDecorator(TrivialDecorator):
    def foo(self):
        return TrivialDecorator.foo(self)

LessTrivialDecorator(None).foo()

will fail with

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in foo
TypeError: 'property' object is not callable

This is a problem because it means that subclasses of subclasses of proxyForInterface classes will need to do different things to access parent class behaviour, depending on which methods their parent overrode.

Change History (22)

comment:1 Changed 14 years ago by Jonathan Lange

This should be done before release so we aren't lugging around another deprecated API.

comment:2 Changed 14 years ago by therve

I gave a try at this, and it doesn't seem very easy. I'd like to propose a change in the workflow here: first create a branch with a new TODO test, with a reference to this ticket. And then try to find a solution.

Of course, it's only valuable if there's not an obvious solution :).

comment:3 Changed 14 years ago by Jonathan Lange

Cc: radix Glyph added

Personally I don't think it's worth making such a branch.

radix, glyph: Got suggestions on how to solve this?

comment:4 Changed 14 years ago by Glyph

In my experience, once one has a failing TODO test, the rest is easy, and there's no point in creating a second branch :).

I'll have a look at how to solve it, though...

comment:5 Changed 14 years ago by Glyph

author: glyph
Branch: branches/proxyForInterface-upcall-2901

(In [21968]) Branching to 'proxyForInterface-upcall-2901'

comment:6 Changed 14 years ago by Glyph

Cc: Jonathan Lange therve added
Keywords: review added
Owner: radix deleted
Priority: normalhighest

Ready for review.

I did it really fast, so maybe I made some mistakes. On the other hand, it was really easy, so maybe I didn't.

comment:7 Changed 14 years ago by Glyph

(In [21970]) re #2901 jml noted that my usage of new-style classes was inconsistent. not a review comment technically, but nevertheless...

comment:8 Changed 14 years ago by therve

Keywords: review removed
Owner: set to Glyph

Damn I wasn't very far from the solution :). Good job.

Things I've spotted:

  • _proxiedClassMethod and _proxyDescriptor should probably be named _ProxiedClassMethod and _ProxyDescriptor
  • attributeName of _proxyDescriptor is undocumented
  • there is probably a missing test for checking that arguments are correctly pass to the _proxiedClassMethod.__call__ method (if I change the signature of __call__ nothing breaks).
  • not new in this branch, but instead of doing for name, description in iface.namesAndDescriptions(), you can probably do for name in iface.names(). Or use the description value :).

This is going to conflict pretty badly with #2895, so this ticket should probably go in first.

comment:9 Changed 14 years ago by Glyph

Status: newassigned

comment:10 Changed 14 years ago by Glyph

Keywords: review added
Owner: Glyph deleted
Status: assignednew

comment:11 Changed 14 years ago by therve

Keywords: review removed
Owner: set to Glyph

Perfect. Please limit the docstring of _ProxiedClassMethod to 80 columns, then merge.

comment:12 Changed 14 years ago by Glyph

Status: newassigned

comment:13 Changed 14 years ago by Glyph

Resolution: fixed
Status: assignedclosed

(In [21987]) Correct the behavior of proxyForInterface with respect to inheritance.

Author: glyph

Reviewer: therve

Fixes #2901

This corrects two bugs in proxyForInterface. The first is that subclasses of types generated for proxyForInterface were not providing convincing facimilies of the methods that they allegedly implemented; you could not define a proxy class Foo and a subclass Bar, and have Bar call Foo.method(self). Now you can.

The other is that proxyies for sub-interfaces were incorrectly restricting access to the methods of their superclasses.

comment:14 Changed 14 years ago by therve

(In [21988]) Revert r21987: missing tests for get code path.

Refs #2901

comment:15 Changed 14 years ago by therve

Resolution: fixed
Status: closedreopened

There is a missing test for type=None in get property.

comment:16 Changed 14 years ago by Glyph

Branch: branches/proxyForInterface-upcall-2901branches/proxyForInterface-upcall-2901-2

(In [21990]) Branching to 'proxyForInterface-upcall-2901-2'

comment:17 Changed 14 years ago by Glyph

(In [21992]) re #2901 do it right

comment:18 Changed 14 years ago by Glyph

(In [21993]) re #2901 test coverage for that case.

comment:19 Changed 14 years ago by Glyph

Keywords: review added
Owner: Glyph deleted
Status: reopenednew

Test added, changeset in previous comment.

comment:20 Changed 14 years ago by therve

Keywords: review removed
Owner: set to Glyph

I tried hard to find something to say, but didn't manage to find anything. Please merge!

comment:21 Changed 14 years ago by Glyph

Resolution: fixed
Status: newclosed

(In [21994]) Correct the behavior of proxyForInterface with respect to inheritance.

(This is a repeat of r21987 with an additional test.)

Author: glyph

Reviewer: therve

Fixes #2901

This corrects two bugs in proxyForInterface. The first is that subclasses of types generated for proxyForInterface were not providing c onvincing facimilies of the methods that they allegedly implemented; you could not define a proxy class Foo and a subclass Bar, and have

Bar call Foo.method(self). Now you can.

The other is that proxyies for sub-interfaces were incorrectly restricting access to the methods of their superclasses.

comment:22 Changed 11 years ago by <automation>

Owner: Glyph deleted
Note: See TracTickets for help on using tickets.