Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

Use standard IPython startup instead of embed #74

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bendavis78
Copy link
Contributor

Fixes #73

@techniq
Copy link
Collaborator

techniq commented Aug 15, 2013

One reason I changed to use embed was it was part of the public API (init.py) and consistent across versions (after 0.10). I agree and understand the reason not to use it (especially coming from an IPython developer). A couple issues I see with the current pull request:

  • IPython 1.x moved the ipapp module from IPython.frontend.terminal to IPython.terminal so we need some more conditional logic to get TerminalIPythonApp imported
  • Issue IPython not using default profile #36 was opened wanting the default profile loaded. I saw in the Django issue that this goes back and forth, and ultimately in issue IPython not using default profile #36 the request was to have the confirm_ext = False. I really don't want to expose another argument in Shell()'s constructor so I think this should just always be set to False (I figure 95%+ would prefer this, if not 100% :) ) or expose it as a Flask config parameter (maybe a key like SCRIPT_IPYTHON_CONFIRM_EXIT) but I don't know at this point if it even needs to be exposed.

@bendavis78
Copy link
Contributor Author

Yeah as for #36 I agree there's no point to add an argument for confirm_ext, when a better way to do that would be via ipython config. The main thing should be to get user configuration loading correctly. I also realized my patch is broken as it doesn't seem to pull in the user context, so I need to figure that out as well. I'll see if I can add conditional support for the various versions also.

@techniq
Copy link
Collaborator

techniq commented Aug 20, 2013

I just got a chance to look to see how Django is handling this and see that IPython 1.x now has a public API IPython.start_ipython() which looks like what we should be using moving forward (in case it's namespace moves again).
https://github.com/django/django/blob/master/django/core/management/commands/shell.py

It also seems like you're doing a lot with the TerminalIPythonApp and TerminalInteractiveShell instances. Is all this needed / is there a cleaning way?

@bendavis78
Copy link
Contributor Author

Unfortunately start_ipython() doesn't take user_ns (yet). See my stackoverflow question here: http://stackoverflow.com/questions/18318257/how-can-i-start-an-ipython-application-instance-while-passing-a-custom-user-ns-a/18345717#18345717

Ideally ipython should be using TerminalIPythonApp. Unfortunately this is the best way I've found to do it while still supporting a custom user_ns and banner.

@jstypka
Copy link

jstypka commented Aug 4, 2015

Seems to work for me

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use standard IPython startup instead of embed
3 participants