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

cleanup of exit/quit commands for qt console #183

Merged
6 commits merged into from Oct 26, 2010
Merged

Conversation

eteq
Copy link
Contributor

@eteq eteq commented Oct 25, 2010

This implements Fernando's suggestion from the list of removing the exit/quit Quitter objects that were previously used in ipython (and moving the quitter to deathrow, as it's used nowhere else), in favor of only an updated %exit magic (still also aliased to quit, Exit, and Quit).

The re-worked exit magic tor the qtconsole also now bypasses the prompt about closing the kernel, and supports a "-k" option that leaves the kernel running but closes the qt window. This also required making some changes for the pure python version of qtconsole, but exit() wasn't working at all previously - it was catching the SystemExit exception the builtin python quit/exit uses.

I also tried merging this branch into another branch tracking the upstream trunk, and got a conflict due to some changes with the qtconsole exit prompt, although it's straightforward to merge. I'm not clear on the correct procedure for the pull request, though - I gather from the mailing list that I'm not supposed to merge in the upstream trunk here, but my git-fu is not very strong, so I'm not sure how I am supposed to include this merged fix... Or do I leave that to whomever accepts the pull request?

@minrk
Copy link
Member

minrk commented Oct 25, 2010

Thanks for this, sorry about the conflict, that's my code in your way. It should indeed be quite a straightforward merge.

You can give trunk a clean merge if you rebase your branch onto master, and handle the conflict there.

Does this specifically link %exit to the requesting frontend's closeEvent, is that how it works? Or are there other things happening as well?

Thanks for this!

@eteq
Copy link
Contributor Author

eteq commented Oct 26, 2010

Ok, I rebased the qt-exiting branch to the current head, so it should now pull without any problem. However, it doesn't appear to be reflected in this pull request... do I have to re-submit the pull request, or do something to directly update the pull request?

@fperez
Copy link
Member

fperez commented Oct 26, 2010

Did you push? I was literally right now reviewing your code and was about to merge it :)

@fperez
Copy link
Member

fperez commented Oct 26, 2010

We can meet briefly on #ipython (freenode IRC) if you want.

@eteq
Copy link
Contributor Author

eteq commented Oct 26, 2010

Well, when I look back at my eteq/ipython/qt-exiting branch, it says the last update was a few minutes ago... but it doesn't seem to be reflected in this pull request. Is it supposed to be automatically?

And might I say, top-notch service for this project - literally withing minutes! :)

@minrk
Copy link
Member

minrk commented Oct 26, 2010

Note that the times of commits don't have to change when you rebase, so you can have a commit from a week ago occur after one from today, if you rebased on top of it.

I just did steps 1-2 of the merge, and it applied cleanly, which it didn't before, so it looks like what's on GH is your rebased version.

@eteq
Copy link
Contributor Author

eteq commented Oct 26, 2010

Fernando pointed out some test suit failures that were due to the added support for hiding builtins that was necessary for the 'exit' and 'quit' python buildins. These are fixed by the commits I just sent up - the test suit now passes, and the branch merges cleanly.

@minrk Ah, I see now - I partly misunderstood how rebase was supposed to work.

@fperez
Copy link
Member

fperez commented Oct 26, 2010

Great job, many thanks! Merging and closing in a second. Welcome to the team :)

This pull request was closed.
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