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

reorganize who knows what about paths #5116

Merged
merged 4 commits into from Feb 22, 2014
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented Feb 13, 2014

  • add NotebookApp.notebook_dir
  • add KernelManager.root_dir
  • remove NotebookManager.notebook_dir, move to FileNBM.notebook_dir

(names of the traitlets are still up for discussion)

Setting NotebookApp.notebook_dir sets KM.root_dir and FileNBM.notebook_dir (99% of cases case), but they can be configured independently.

SessionManager passes the API path to KernelManager, which is responsible for turning it into the kernel's cwd.

Finishes up work started in #4925

@minrk minrk added this to the 2.0 milestone Feb 20, 2014
@@ -202,8 +202,11 @@ def init_handlers(self, settings):
handlers.extend(load_handlers('services.clusters.handlers'))
handlers.extend(load_handlers('services.sessions.handlers'))
handlers.extend(load_handlers('services.nbconvert.handlers'))
handlers.extend([
(r"/files/(.*)", AuthenticatedFileHandler, {'path' : settings['notebook_manager'].notebook_dir}),
# FIXME: /files/ should be handled by the Contents service when it exists
Copy link
Member

Choose a reason for hiding this comment

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

But the contents web service won't send back raw files - it will send back the JSON model of the file in the same way that GitHub contents or the notebook web service does. I think that means that we can't use the contents web service for /files/.

Copy link
Member Author

Choose a reason for hiding this comment

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

The ContentsManager should be able to handle it - it would be a different handler than the '/api/' handler, but the same object turning paths into files. The relevant piece is that contents (generic files) should be responsible rather than the notebook manager, which is currently taking care of both.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, yes I agree.

@ellisonbg
Copy link
Member

OK I have looked through this and left some inline comments. I have also tested it to make sure the cwd of the kernel is set properly as you walk the dashboard around the directory tree.

- add NotebookApp.notebook_dir
- add KernelManager.root_dir
- remove NotebookManager.notebook_dir, move to FileNBM.notebook_dir

Default value for KM.root_dir and fNBM.notebook_dir is NotebookApp.notebook_dir, but they can be configured separately.

SessionManager passes the API path to KernelManager,
which is responsible for turning it into the kernel's cwd.
@ellisonbg
Copy link
Member

Looks good, merging.

@ellisonbg
Copy link
Member

Tested interactively, works fine. Merging.

ellisonbg added a commit that referenced this pull request Feb 22, 2014
reorganize who knows what about paths
@ellisonbg ellisonbg merged commit 6b11b35 into ipython:master Feb 22, 2014
@minrk minrk deleted the os_path branch March 31, 2014 23:36
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
reorganize who knows what about paths
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

2 participants