Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

cleanup IPython handler settings #3088

Merged
merged 5 commits into from Apr 25, 2013

Conversation

Projects
None yet
2 participants
Owner

minrk commented Mar 27, 2013

move any settings / shared objects to the tornado settings dict, rather than a mixture of tornado settings and application references, and application.ipython_app references.

Removes any reference to application / ipython_app attributes in the handlers, in favor of the tornado settings dict. These were a massive pain for anyone who might want to re-use our handlers.

This depends on PR #3011, it is actually just one commit (at this point).

@minrk minrk referenced this pull request Mar 27, 2013

Merged

add stdin to the notebook #3089

Owner

ellisonbg commented Apr 12, 2013

I have skimmed over this and it looks good. Let's wait until #3011 is merged, check for rebase and the I will do a final review pass.

@minrk minrk cleanup IPython handler settings
move settings to the tornado settings dict,
rather than a mixture of tornado settings and application references,
and application.ipython_app references.

removes any reference to application / ipython_app attributes in the handlers,
in favor of the tornado settings dict.
These were a massive pain for anyone who might want to re-use our handlers.
7334b68

@ellisonbg ellisonbg commented on the diff Apr 25, 2013

IPython/frontend/html/notebook/notebookapp.py
super(NotebookWebApplication, self).__init__(new_handlers, **settings)
- self.kernel_manager = kernel_manager
- self.notebook_manager = notebook_manager
- self.cluster_manager = cluster_manager
- self.ipython_app = ipython_app
- self.read_only = self.ipython_app.read_only
- self.config = self.ipython_app.config
- self.use_less = self.ipython_app.use_less
- self.log = log
@ellisonbg

ellisonbg Apr 25, 2013

Owner

Looks like log didn't make it into settings. Do you think we don't need it?

@minrk

minrk Apr 25, 2013

Owner

Well, we never have used it in the handlers. Tornado has its own logging mechanisms, and tornado handlers should probably use them. If anything, the main app should hook up tornado's logging to our app's logging.

@ellisonbg ellisonbg commented on the diff Apr 25, 2013

IPython/frontend/html/notebook/notebookapp.py
@@ -301,10 +308,13 @@ class NotebookApp(BaseIPythonApplication):
kernel_argv = List(Unicode)
- log_level = Enum((0,10,20,30,40,50,'DEBUG','INFO','WARN','ERROR','CRITICAL'),
@ellisonbg

ellisonbg Apr 25, 2013

Owner

It looks like this is inherited from the base class?

@minrk

minrk Apr 25, 2013

Owner

yes, this was a duplicate that I removed. As far as I could tell, the only thing it was used for was changing the default level, which I did below.

Owner

ellisonbg commented Apr 25, 2013

This looks great, merging.

minrk added some commits Apr 25, 2013

@minrk minrk define clear_cookie on websocket handler
as a no-op
3ac44d1
@minrk minrk hook up tornado 3's loggers to our handlers 337bbce
@minrk minrk hook up proper loggers
use IPython logger as first choice, fall back on tornado logger
(for use in non-IPython apps).
bbb160f
@minrk minrk remove unused zmqhttp 4450b19

@ellisonbg ellisonbg added a commit that referenced this pull request Apr 25, 2013

@ellisonbg ellisonbg Merge pull request #3088 from minrk/nbsettings
cleanup IPython handler settings
dd76a23

@ellisonbg ellisonbg merged commit dd76a23 into ipython:master Apr 25, 2013

1 check was pending

default The Travis build is in progress
Details

@minrk minrk deleted the minrk:nbsettings branch Mar 31, 2014

@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

@ellisonbg ellisonbg Merge pull request #3088 from minrk/nbsettings
cleanup IPython handler settings
aefc175
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment