Opened 12 years ago

Closed 11 years ago

#11003 closed enhancement (fixed)

Improve support for lazy_import inside classes

Reported by: Nicolas M. Thiéry Owned by: Jason Grout
Priority: major Milestone: sage-5.0
Component: misc Keywords: Cernay2012
Cc: Rishikesh, Jason Grout, Robert Miller, rbradshaw Merged in: sage-5.0.beta4
Authors: Mike Hansen Reviewers: Nicolas M. Thiéry, Florent Hivert
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Florent Hivert)

Currently, one can do:

    sage: class foo(object):
    ...       bar = LazyImport('sage.bar', 'bar')
    sage: foo.bar
    bar

and this works (assuming sage.bar.bar exists of course). As a syntactic sugar, it would be nice to have lazy_import work as well, if possible with the automatic back-insertion of the object into the class after the actual import occurred:

    sage: class foo(object):
    ...       lazy_import('sage.bar', 'bar')
    sage: type(foo.bar)
    <type 'sage.misc.lazy_import.LazyImport'>
    sage: foo.bar
    bar
    sage: type(foo.bar)
    <type 'bar_type>

Apply: trac_11003-folded.patch

Attachments (2)

trac_11003.patch (4.1 KB) - added by Mike Hansen 11 years ago.
trac_11003-folded.patch (5.3 KB) - added by Florent Hivert 11 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 11 years ago by Mike Hansen

Authors: Mike Hansen
Status: newneeds_review

I think this should be good, but the only thing I have questions about is the switch from f_globals to f_locals.

comment:2 Changed 11 years ago by Nicolas M. Thiéry

Status: needs_reviewneeds_work

Thanks Mike!

What's the rationale for not always walking through the whole mro?

I would suggest to replace walk_owner_namespaces=False by owner=None.

Otherwise, this looks overall looks good. I have no clue at this point about the f_globals vs f_locals point though.

Cheers,

Nicolas

comment:3 in reply to:  2 ; Changed 11 years ago by Mike Hansen

Replying to nthiery:

What's the rationale for not always walking through the whole mro?

Do you mean the break?

comment:4 in reply to:  3 Changed 11 years ago by Nicolas M. Thiéry

Replying to mhansen:

Replying to nthiery:

What's the rationale for not always walking through the whole mro?

Do you mean the break?

Sorry, I should have been more explicit. It's about the using owner.__bases__' rather than `owner.mro.

By the way, what about using owner.mro() to respect encapsulation? Speed is not an issue here, right?

comment:5 Changed 11 years ago by Mike Hansen

Status: needs_workneeds_review

I've updated the patch to fix these two issues.

comment:6 Changed 11 years ago by Nicolas M. Thiéry

Status: needs_reviewneeds_work

Thanks!

It seems the builtin inspect.mro should do the job of supporting both old style and new style classes. Also, just for test-completeness, could you add a test with a lazy imported method in a super class of a class?

Cheers,

Nicolas

Changed 11 years ago by Mike Hansen

Attachment: trac_11003.patch added

comment:7 Changed 11 years ago by Mike Hansen

Status: needs_workneeds_review

inspect.getmro() was what I was looking for and couldn't find last night. Thanks.

comment:8 Changed 11 years ago by Nicolas M. Thiéry

Reviewers: Nicolas M. Thiéry

Hi Mike!

I just pushed a little reviewer's patch on the Sage-Combinat queue. If you are happy with it, please fold, post, and set a positive review on my behalf.

Cheers,

Nicolas

comment:9 Changed 11 years ago by Mike Hansen

Cc: rbradshaw added

Robert Bradshaw is here, and we're going to discuss the f_globals to f_locals change.

comment:10 in reply to:  9 Changed 11 years ago by Nicolas M. Thiéry

Hi!

Replying to mhansen:

Robert Bradshaw is here, and we're going to discuss the f_globals to f_locals change.

What was the result of this discussion?

Shall we get this patch in?

Cheers,

Nicolas

Changed 11 years ago by Florent Hivert

Attachment: trac_11003-folded.patch added

comment:11 in reply to:  8 Changed 11 years ago by Florent Hivert

Status: needs_reviewpositive_review

I just pushed a little reviewer's patch on the Sage-Combinat queue. If you are happy with it, please fold, post, and set a positive review on my behalf.

I'm ok with the review patch. => Positive review.

comment:12 Changed 11 years ago by Florent Hivert

Description: modified (diff)
Reviewers: Nicolas M. ThiéryNicolas M. Thiéry, Florent Hivert

comment:13 Changed 11 years ago by Nicolas M. Thiéry

Keywords: Cernay2012 added
Milestone: sage-wishlistsage-5.0

comment:14 Changed 11 years ago by Jeroen Demeyer

Merged in: sage-5.0.beta4
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.