Skip to content

Rewrite quit()/exit()/Quit()/Exit() calls as magic #353

Closed
wants to merge 2 commits into from

3 participants

@dwf
dwf commented Apr 10, 2011

This is a resubmit of #345, GitHub wasn't updating the range, perhaps because I pushed those changes while the pull request was closed. #352 was an alternate plan that was decided against.

This attempts to resolve the complaint in #142 about quit() and exit() and their capitalized counterparts, by explicitly checking for these forms (with a regexp, for speed) and rewriting them as their corresponding magics. Just checks for at least one pair of parens following the initial token, with anything in between; nothing fancy. See the commit message of d0e797b for details.

@takluyver
IPython member

Rather than integrating into AutoMagicChecker, would it be tidier to add a QuitterChecker to complement the QuitterHandler you've added?

@dwf
dwf commented Apr 10, 2011

I don't know. Checkers and handlers don't necessarily have a 1:1 mapping, since both MagicChecker and AutoMagicChecker yield MagicHandlers, for example.

On the other hand, maybe quit(), etc. should work even with automagic off.

@takluyver
IPython member

Mhm, but it seems to me that you're largely running a separate execution path through the same code, using is_quitter, so I think it would be neater to separate that out.

@dwf
dwf commented Apr 10, 2011

I've refactored in the manner you suggested. QuitterChecker is now it's own separate thing.

@dwf dwf Add quitter checker and handler for quit(), etc. calls. Closes gh-142.
After some discussion with Fernando it was decided that detecting calls
on all magics as in pull request #352 was probably overkill. Opted for
detecting with a regex possible calls to absent Quitter objects and
rewriting them (as before), with visual notification of this rewrite.
This is done with a QuitterChecker which has priority just below that
of AutoMagicChecker.

The only case where you will get a NameError now is if you input
newlines before the starting and closing paren, but that's sufficiently
crazy as to not be worth considering (it takes one, optional,
integer argument in the real Python shell, why you'd ever need
newlines...).

Also subclassed MagicHandler to avoid doing the regex check twice (once
in the Checker and once in the Handler) and factored out the creation of
the replacement command so as to avoid potential code rot.
d0e797b
@takluyver
IPython member

OK, thanks, that's neater. I've done a brief test, and I think it's an improvement. One more quick thing: can you add an automated test, in core/tests/test_handlers? It should be as simple as a single tuple.

@dwf dwf Tests for the quitter handler.
Also amended the tester to run within the builtin trap context manager
so that the builtins don't interfere.
efbb04f
@dwf
dwf commented Apr 10, 2011

Done. It was a little more complicated, some of the tests were failing and it was because the builtin_trap wasn't being used for that prefilter.

@takluyver
IPython member

Thanks, and well done for sorting that little issue with the tests. @fperez: I think this is ready to merge.

@fperez
IPython member
fperez commented Apr 10, 2011

Hey, let's hold off for a minute... Last night I was thinking about this, and I have an idea. Give me an hour to see if it works, and if it doesn't we'll merge David's solution.

@takluyver
IPython member

David, I'm sorry, I've encouraged you to jump through all these hoops, and now talking on IRC, Fernando and I have devised a simpler way to achieve the same thing, which I'm about to implement. But it wouldn't have happened at all without you starting to work on it, so thank-you for tackling the issue, and please do keep contributing.

@fperez
IPython member
@dwf
dwf commented Apr 11, 2011
@fperez
IPython member
fperez commented Apr 11, 2011

Sure, just see #357 for the new version.

@fperez
IPython member
fperez commented Apr 11, 2011

Closing this one, since we'll go for the cleaner solution in #357.

@fperez fperez closed this Apr 11, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.