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

allow multiple instances of a Magic #1964

Merged
merged 2 commits into from Jun 20, 2012
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented Jun 15, 2012

as reported on-list, Magics were using a class magics mapping, bound to the first instance, rather than creating a mapping for each instance.

This may have been by design, but it offers little-to-no benefit, and surprising unchangeable behavior in (admittedly unlikely) cases.

the true magics table is now an *instance* attribute, rather than a *class* attribute, which was previously being clobbered with references to the first instance.
@fperez
Copy link
Member

fperez commented Jun 15, 2012

More than by design, it was by expedience in the rush of getting everything done. Definitely this needs a cleanup, I've already thought of other cases where the current implementation will give us headaches.

@minrk
Copy link
Member Author

minrk commented Jun 15, 2012

Well, given all the cell magics people have been contributed, it's clearly highly valuable and great work. But it is pretty complex, so we are going to find little weird bugs like this while we are putting it through the ringer.

@ivanov
Copy link
Member

ivanov commented Jun 15, 2012

mark this with @dec.skip_known_failure to just merge and move on, or is the intent for this to be a work-in-progress?

@minrk
Copy link
Member Author

minrk commented Jun 15, 2012

oops, no - there should be two commits, test and fix. I just forgot to push the second :)

@minrk
Copy link
Member Author

minrk commented Jun 19, 2012

Test results for commit 139d6c0 (can't merge cleanly)
Platform: darwin

  • python2.6: OK (libraries not available: cython matplotlib oct2py pygments pymongo qt rpy2 tornado wx wx.aui)
  • python2.7: OK (libraries not available: oct2py wx wx.aui)
  • python3.2: OK (libraries not available: cython matplotlib oct2py pymongo qt rpy2 wx wx.aui)

Not available for testing: python3.1

@minrk
Copy link
Member Author

minrk commented Jun 20, 2012

Any reason this shouldn't be merged? It's relatively minor, so I would be okay letting it slip to after 0.13, but the side effects of the bug are quite surprising when they come up (as seen on-list).

@fperez
Copy link
Member

fperez commented Jun 20, 2012

No, no reason, just too busy. I'd had a first look before and just didn't finish, sorry.

This is a good fix for a wart from the cell magics work, so it should go in. Thanks for the good work!

Merging now.

fperez added a commit that referenced this pull request Jun 20, 2012
allow multiple instances of a Magic by tracking all instances of any Magics subclass rather than having a mapping that only tracks the first instance.
@fperez fperez merged commit 6331c3d into ipython:master Jun 20, 2012
@minrk minrk deleted the magics_class branch March 31, 2014 23:36
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
allow multiple instances of a Magic by tracking all instances of any Magics subclass rather than having a mapping that only tracks the first instance.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants