Opened 10 years ago

Closed 10 years ago

#8250 closed enhancement (fixed)

Extend ClasscallMetaclass to allow for binding behavior

Reported by: nthiery Owned by: nthiery
Priority: major Milestone: sage-4.3.3
Component: categories Keywords: ClassCall, descriptor, __classget__
Cc: sage-combinat Merged in: sage-4.3.3.alpha1
Authors: Nicolas M. Thiéry Reviewers: Florent Hivert
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

From the documentation:

(using this patch) we show how to implement a nested class Foo.Bar with a binding behavior, as if it was a method of Foo: namely for foo an instance of Foo, calling foo.Bar() is equivalent to Foo.Bar(foo)::

            sage: import functools
            sage: from sage.misc.classcall_metaclass import ClasscallMetaclass
            sage: class Foo:
            ...       class Bar(object):
            ...           __metaclass__ = ClasscallMetaclass
            ...           @staticmethod
            ...           def __classget__(cls, instance, owner):
            ...               print "calling __classget__"
            ...               if instance is None:
            ...                   return cls
            ...               return functools.partial(cls, instance)
            ...           def __init__(self, instance):
            ...               self.instance = instance
            sage: foo = Foo()
            sage: bar = foo.Bar()
            calling __classget__
            sage: bar.instance == foo
            True

This will be used by the upcoming improvements to the functorial constructions in categories

Attachments (2)

trac_8250-classcall-classget.patch (9.9 KB) - added by nthiery 10 years ago.
Documentation improvements after phone discussion with Florent
trac_8250-classcall-classget-review-2.patch (3.8 KB) - added by hivert 10 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 10 years ago by hivert

This is a check to see if CC to sage-combinat-commits is working. Apologies for the trouble.

Cheers,

Florent

comment:2 Changed 10 years ago by nthiery

  • Status changed from new to needs_review

comment:3 follow-up: Changed 10 years ago by hivert

Hi Nicolas,

They are a few problem with this patch:

  • it seems that line 119-125 (in the file) does not belongs to there ! They are after a return. Are they a bad copy paste ?
  • I think it would be more natural to add this feature to NestedClassMetaclass rather than to ClasscallMetaclass
  • It took me half an hour to fully understand what's happening. In particular the doc is wrong in saying that
    This method implements a binding behavior for ``foo.cls`` by delegating it to ``cls.__classget__(foo)``
    

indeed cls.classget(foo, Foo) is called. Can you confirm this ?

Florent

comment:4 Changed 10 years ago by hivert

  • Status changed from needs_review to needs_work

comment:5 in reply to: ↑ 3 ; follow-up: Changed 10 years ago by nthiery

Replying to hivert:

Hi Nicolas,

They are a few problem with this patch:

  • it seems that line 119-125 (in the file) does not belongs to there ! They are after a return. Are they a bad copy paste ?

Thanks for spotting this; that could explain some trouble I was having right now :-)

  • I think it would be more natural to add this feature to NestedClassMetaclass rather than to ClasscallMetaclass

Let's discuss this over the phone.

  • It took me half an hour to fully understand what's happening. In particular the doc is wrong in saying that
    This method implements a binding behavior for ``foo.cls`` by delegating it to ``cls.__classget__(foo)``
    

indeed cls.classget(foo, Foo) is called. Can you confirm this ?

Oops right. It calls classget as per the descriptor protocol (which includes a 3rd argument).

Please let me know if you already made a patch on top on this one in the queue.

comment:6 in reply to: ↑ 5 Changed 10 years ago by hivert

Replying to nthiery:

  • I think it would be more natural to add this feature to NestedClassMetaclass rather than to ClasscallMetaclass

Let's discuss this over the phone.

Ok.

Please let me know if you already made a patch on top on this one in the queue.

please see trac_8250-classcall-classget-review-fh.patch in combinat's queue. I'll upload it there as soon as we decided to move to NestedClassMetaclass or not.

Changed 10 years ago by nthiery

Documentation improvements after phone discussion with Florent

comment:7 Changed 10 years ago by nthiery

  • Status changed from needs_work to needs_review

comment:8 Changed 10 years ago by hivert

There were still a few problems with the documentation:

  • references to methods __classget__ and __classcall__ instead of __get__ and __call__;
  • Missing title for the file;
  • Missing use of NestedClassMetaclass in the outer class resulting in non standard names for the class. Moreover, the example now demonstrate the correct usage.
  • I also added these two metaclasses to the reference manual. Nicolas: can you confirm that we want it ?

I uploaded a review patch. Please review :-)

comment:9 Changed 10 years ago by hivert

As we spoke on the phone I added a comment and corrected a typo. Ready for review. I'm ok with your patch and if you are ok with mine you can put positive review.

comment:10 Changed 10 years ago by nthiery

  • Status changed from needs_review to positive_review

comment:11 Changed 10 years ago by mvngu

  • Merged in set to sage-4.3.3.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.