Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend ClasscallMetaclass to allow for binding behavior #8250

Closed
nthiery opened this issue Feb 12, 2010 · 9 comments
Closed

Extend ClasscallMetaclass to allow for binding behavior #8250

nthiery opened this issue Feb 12, 2010 · 9 comments

Comments

@nthiery
Copy link
Contributor

nthiery commented Feb 12, 2010

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

CC: @sagetrac-sage-combinat

Component: categories

Keywords: ClassCall, descriptor, classget

Author: Nicolas M. Thiéry

Reviewer: Florent Hivert

Merged: sage-4.3.3.alpha1

Issue created by migration from https://trac.sagemath.org/ticket/8250

@nthiery nthiery added this to the sage-4.3.3 milestone Feb 12, 2010
@nthiery nthiery self-assigned this Feb 12, 2010
@hivert
Copy link

hivert commented Feb 12, 2010

comment:1

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

Cheers,

Florent

@hivert
Copy link

hivert commented Feb 12, 2010

comment:3

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

@nthiery
Copy link
Contributor Author

nthiery commented Feb 12, 2010

comment:5

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.

@hivert
Copy link

hivert commented Feb 13, 2010

comment:6

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.

@nthiery
Copy link
Contributor Author

nthiery commented Feb 13, 2010

Attachment: trac_8250-classcall-classget.patch.gz

Documentation improvements after phone discussion with Florent

@hivert
Copy link

hivert commented Feb 13, 2010

comment:8

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

@hivert
Copy link

hivert commented Feb 13, 2010

comment:9

Attachment: trac_8250-classcall-classget-review-2.patch.gz

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.

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Feb 15, 2010

Merged: sage-4.3.3.alpha1

@sagetrac-mvngu
Copy link
Mannequin

sagetrac-mvngu mannequin commented Feb 15, 2010

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants