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

%xdel magic #419

Merged
merged 4 commits into from May 29, 2011
Merged

%xdel magic #419

merged 4 commits into from May 29, 2011

Conversation

takluyver
Copy link
Member

Fernando suggested this on issue #141. The %xdel magic hunts out the references to an object in the IPython machinery, removing them so that the object can be released from memory. I'm fairly satisfied with the implementation, although please tell me if I've overlooked somewhere that we can keep references.

The trickiest bit of this was writing the test. We have to eliminate extra references kept by the testing framework (fair enough). Then it still doesn't work unless I print the test function's frame's local variables before checking that the object was released. I've no idea what difference that makes (and calling gc.collect() doesn't work instead), but I've put it in until we can find a better way to make the test work. Obviously please make sure that this passes on other systems.

I've also manually checked it interactively, which works without any extra trickery.

@takluyver
Copy link
Member Author

OK, I've found a better way to make the test pass now, by modifying __delitem__ for the user namespace the test suite uses.

namespace, and delete references to it.
"""
if varname in ('__builtin__', '__builtins__'):
raise ValueError("Refusing to delete %s" % varname)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change the message to:

"Refusing to delete %s from builtins"

so it's clearer to the user what happened.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is in case of someone doing xdel __builtins__, not trying to delete something from the builtins.

At present, trying to xdel something in the builtins just gives NameError: name 'eval' is not defined (because it's looking in the user namespace). We could check for that case separately.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry. Then:

  • you're right, the message is OK for this case
  • yes, I think we should do what I thought the code was doing. Deleting stuff from builtins is just going to blow lots of things up, so let's not do it in xdel.

Other than that, good to go, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have been clearer: xdel will not delete things from the builtins - it doesn't even look at them. The question is simply whether we want toc check that case and provide a specific error message, rather than the general NameError message it gives for any variable not in user_ns.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, OK. Then I wouldn't worry about it, leave it as-is and merge away, it's such a rare edge case it's not worth losing more sleep over it. Thanks!

@fperez
Copy link
Member

fperez commented May 26, 2011

Other than my minor comments, this looks great, thanks! I ran the tests and did some more interactive checking, and the functionality looks exactly like what we wanted. Awesome.

Once you make those tiny fixes, merge away!

@takluyver takluyver merged commit e971e2f into ipython:master May 29, 2011
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

2 participants