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

Make the TerminalInteractiveShell class configurable. #10373

Merged
merged 1 commit into from
Apr 10, 2017

Conversation

Carreau
Copy link
Member

@Carreau Carreau commented Mar 2, 2017

Should allow custom frontend in separate packages.

cc @takluyver, @ivanov


That's sufficient for me to get the basics working.

rlipython

@Carreau Carreau force-pushed the tis-configurable branch from fd0ceb6 to a116ddb Compare March 2, 2017 23:24
@Carreau Carreau modified the milestone: 6.0 Mar 2, 2017
@takluyver
Copy link
Member

As I said in the other issue, I wouldn't do this - I would subclass TerminalIPythonApp in rlipython and override init_shell there, so you start it with rlipython, not by configuring ipython.

@Carreau
Copy link
Member Author

Carreau commented Mar 3, 2017

As I said in the other issue, I wouldn't do this - I would subclass TerminalIPythonApp in rlipython and override init_shell there, so you start it with rlipython

That implies that all IPython wrapers and extensions that exists need to be updated to take an option instead of allowing it to be done globally. That seem like quite the task, and the annoyance for users.

@ivanov
Copy link
Member

ivanov commented Mar 3, 2017

As I said in the other issue, I wouldn't do this - I would subclass TerminalIPythonApp in rlipython and override init_shell there, so you start it with rlipython, not by configuring ipython.

Why weld something when it could be swappable?

Please consider why we're doing this in the first place: IPython 5.0 broke a bunch of people's workflows when it unilaterally moved away from readline to prompt toolkit without giving users the ability to fall back to old behavior. #10364 has a list of many of them, and proposed bringing back that code into IPython, which you (@takluyver) are strongly against. @Carreau is already making the compromise of not putting that code back into IPython, but instead proposes a straightforward ability for users who relied on the old behavior to install a package, set a config, and continue using IPython.

interactive_shell_class = Type(
klass=object, # use default_value otherwise which only allow subclasses.
default_value=TerminalInteractiveShell,
help="Class to use to instantiate the TerminalInteractiveShell object. Usefull for custom Frontends"
Copy link
Member

Choose a reason for hiding this comment

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

"Useful"

Should allow custom frontend in separate packages.
@Carreau Carreau merged commit c2b5745 into ipython:master Apr 10, 2017
@Carreau Carreau modified the milestones: 5.4, 6.0 Apr 10, 2017
@Carreau
Copy link
Member Author

Carreau commented Apr 10, 2017

@meeseeksdev backport

@lumberbot-app
Copy link
Contributor

lumberbot-app bot commented Apr 10, 2017

Oops, something went wrong applying the patch... Please have a look at my logs.

@Carreau Carreau added the Still Needs Manual Backport Added My MrMeeseeks when a backport fails. Help by backporting it, solving conflicts, send PR. label Apr 10, 2017
@Carreau Carreau deleted the tis-configurable branch April 18, 2017 16:31
Carreau added a commit to Carreau/ipython that referenced this pull request Apr 18, 2017
Make the TerminalInteractiveShell class configurable.
Carreau added a commit that referenced this pull request Apr 20, 2017
@Carreau Carreau added backported PR that have been backported by MrMeeseeks and removed Still Needs Manual Backport Added My MrMeeseeks when a backport fails. Help by backporting it, solving conflicts, send PR. labels May 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported PR that have been backported by MrMeeseeks help wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants