address review in contents service

- various docstrings, comments clarified and updated
- misc typos
- fix and test creating an untitled directory via POST
- only define `message` if there's something to say

IPython/html/notebook/handlers.py

@@ -17,7 +17,7 @@ from ..utils import url_escape
 class NotebookHandler(IPythonHandler):
 
     @web.authenticated
-    def get(self, path=''):
+    def get(self, path):
         """get renders the notebook template if a name is given, or 
         redirects to the '/files/' handler if the name is not given."""
         path = path.strip('/')

IPython/html/notebookapp.py

@@ -187,7 +187,9 @@ class NotebookWebApplication(web.Application):
         return settings
 
     def init_handlers(self, settings):
-        # Load the (URL pattern, handler) tuples for each component.
+        """Load the (URL pattern, handler) tuples for each component."""
+        
+        # Order matters. The first handler to match the URL will handle the request.
         handlers = []
         handlers.extend(load_handlers('tree.handlers'))
         handlers.extend(load_handlers('auth.login'))
@@ -205,6 +207,7 @@ class NotebookWebApplication(web.Application):
         handlers.append(
             (r"/nbextensions/(.*)", FileFindHandler, {'path' : settings['nbextensions_path']}),
         )
+        # register base handlers last
         handlers.extend(load_handlers('base.handlers'))
         # set the URL that will be redirected from `/`
         handlers.append(

IPython/html/services/contents/filemanager.py

@@ -61,9 +61,8 @@ class FileContentsManager(ContentsManager):
         except OSError as e:
             self.log.debug("copystat on %s failed", dest, exc_info=True)
 
-    def _get_os_path(self, path=''):
-        """Given a filename and API path, return its file system
-        path.
+    def _get_os_path(self, path):
+        """Given an API path, return its file system path.
 
         Parameters
         ----------
@@ -131,8 +130,8 @@ class FileContentsManager(ContentsManager):
             Whether the file exists.
         """
         path = path.strip('/')
-        nbpath = self._get_os_path(path)
-        return os.path.isfile(nbpath)
+        os_path = self._get_os_path(path)
+        return os.path.isfile(os_path)
 
     def exists(self, path):
         """Returns True if the path exists, else returns False.
@@ -167,7 +166,6 @@ class FileContentsManager(ContentsManager):
         model['created'] = created
         model['content'] = None
         model['format'] = None
-        model['message'] = None
         return model
 
     def _dir_model(self, path, content=True):
@@ -191,12 +189,16 @@ class FileContentsManager(ContentsManager):
         model['type'] = 'directory'
         if content:
             model['content'] = contents = []
-            for os_path in glob.glob(self._get_os_path('%s/*' % path)):
-                name = os.path.basename(os_path)
+            os_dir = self._get_os_path(path)
+            for name in os.listdir(os_dir):
+                os_path = os.path.join(os_dir, name)
                 # skip over broken symlinks in listing
                 if not os.path.exists(os_path):
                     self.log.warn("%s doesn't exist", os_path)
                     continue
+                elif not os.path.isfile(os_path) and not os.path.isdir(os_path):
+                    self.log.debug("%s not a regular file", os_path)
+                    continue
                 if self.should_list(name) and not is_hidden(os_path, self.root_dir):
                     contents.append(self.get_model(
                         path='%s/%s' % (path, name),
@@ -217,6 +219,9 @@ class FileContentsManager(ContentsManager):
         model['type'] = 'file'
         if content:
             os_path = self._get_os_path(path)
+            if not os.path.isfile(os_path):
+                # could be FIFO
+                raise web.HTTPError(400, "Cannot get content of non-file %s" % os_path)
             with io.open(os_path, 'rb') as f:
                 bcontent = f.read()
             try:
@@ -407,14 +412,14 @@ class FileContentsManager(ContentsManager):
         old_os_path = self._get_os_path(old_path)
 
         # Should we proceed with the move?
-        if os.path.isfile(new_os_path):
-            raise web.HTTPError(409, u'File already exists: %s' % new_os_path)
+        if os.path.exists(new_os_path):
+            raise web.HTTPError(409, u'File already exists: %s' % new_path)
 
         # Move the file
         try:
             shutil.move(old_os_path, new_os_path)
         except Exception as e:
-            raise web.HTTPError(500, u'Unknown error renaming file: %s %s' % (old_os_path, e))
+            raise web.HTTPError(500, u'Unknown error renaming file: %s %s' % (old_path, e))
 
         # Move the checkpoints
         old_checkpoints = self.list_checkpoints(old_path)
@@ -462,6 +467,8 @@ class FileContentsManager(ContentsManager):
     def create_checkpoint(self, path):
         """Create a checkpoint from the current state of a file"""
         path = path.strip('/')
+        if not self.file_exists(path):
+            raise web.HTTPError(404)
         src_path = self._get_os_path(path)
         # only the one checkpoint ID:
         checkpoint_id = u"checkpoint"
@@ -521,7 +528,7 @@ class FileContentsManager(ContentsManager):
     def get_kernel_path(self, path, model=None):
         """Return the initial working dir a kernel associated with a given notebook"""
         if '/' in path:
-            os_dir = path.rsplit('/', 1)[0]
+            parent_dir = path.rsplit('/', 1)[0]
         else:
-            os_dir = ''
-        return self._get_os_path(os_dir)
+            parent_dir = ''
+        return self._get_os_path(parent_dir)

IPython/html/services/contents/handlers.py

@@ -73,14 +73,11 @@ class ContentsHandler(IPythonHandler):
         model = self.get_json_body()
         if model is None:
             raise web.HTTPError(400, u'JSON body missing')
-        print('before', model)
         model = cm.update(model, path)
-        print('after', model)
         self._finish_model(model)
 
     def _copy(self, copy_from, copy_to=None):
-        """Copy a file, optionally specifying the new path.
-        """
+        """Copy a file, optionally specifying a target directory."""
         self.log.info(u"Copying {copy_from} to {copy_to}".format(
             copy_from=copy_from,
             copy_to=copy_to or '',
@@ -96,13 +93,17 @@ class ContentsHandler(IPythonHandler):
         self.set_status(201)
         self._finish_model(model)
 
-    def _create_empty_file(self, path, ext='.ipynb'):
-        """Create an empty file in path
-
-        If name specified, create it in path.
+    def _new(self, path, type='notebook', ext=''):
+        """Create an empty file or directory in directory specified by path
+        
+        ContentsManager picks the filename.
         """
-        self.log.info(u"Creating new file in %s", path)
-        model = self.contents_manager.new(path=path, ext=ext)
+        self.log.info(u"Creating new %s in %s", type or 'file', path)
+        if type:
+            model = {'type': type}
+        else:
+            model = None
+        model = self.contents_manager.new(model, path=path, ext=ext)
         self.set_status(201)
         self._finish_model(model)
 
@@ -115,9 +116,9 @@ class ContentsHandler(IPythonHandler):
     @web.authenticated
     @json_errors
     def post(self, path=''):
-        """Create a new file or directory in the specified path.
+        """Create a new file in the specified path.
 
-        POST creates new files or directories. The server always decides on the name.
+        POST creates new files. The server always decides on the name.
 
         POST /api/contents/path
           New untitled, empty file or directory.
@@ -129,7 +130,7 @@ class ContentsHandler(IPythonHandler):
         cm = self.contents_manager
 
         if cm.file_exists(path):
-            raise web.HTTPError(400, "Cannot POST to existing files, use PUT instead.")
+            raise web.HTTPError(400, "Cannot POST to files, use PUT instead.")
 
         if not cm.dir_exists(path):
             raise web.HTTPError(404, "No such directory: %s" % path)
@@ -138,13 +139,14 @@ class ContentsHandler(IPythonHandler):
 
         if model is not None:
             copy_from = model.get('copy_from')
-            ext = model.get('ext', '.ipynb')
+            ext = model.get('ext', '')
+            type = model.get('type')
             if copy_from:
                 self._copy(copy_from, path)
             else:
-                self._create_empty_file(path, ext=ext)
+                self._new(path, type=type, ext=ext)
         else:
-            self._create_empty_file(path)
+            self._new(path)
 
     @web.authenticated
     @json_errors
@@ -168,7 +170,7 @@ class ContentsHandler(IPythonHandler):
             else:
                 self._upload(model, path)
         else:
-            self._create_empty_file(path)
+            self._new(path)
 
     @web.authenticated
     @json_errors

IPython/html/services/contents/manager.py

@@ -119,7 +119,7 @@ class ContentsManager(LoggingConfigurable):
         raise NotImplementedError('must be implemented in a subclass')
 
     def exists(self, path):
-        """Does a file or directory exist at the given name and path?
+        """Does a file or directory exist at the given path?
 
         Like os.path.exists
 
@@ -181,7 +181,11 @@ class ContentsManager(LoggingConfigurable):
         return "Serving contents"
 
     def get_kernel_path(self, path, model=None):
-        """ Return the path to start kernel in """
+        """Return the API path for the kernel
+        
+        KernelManagers can turn this value into a filesystem path,
+        or ignore it altogether.
+        """
         return path
 
     def increment_filename(self, filename, path=''):
@@ -204,7 +208,7 @@ class ContentsManager(LoggingConfigurable):
         for i in itertools.count():
             name = u'{basename}{i}{ext}'.format(basename=basename, i=i,
                                                 ext=ext)
-            if not self.file_exists(u'{}/{}'.format(path, name)):
+            if not self.exists(u'{}/{}'.format(path, name)):
                 break
         return name
 
@@ -218,22 +222,35 @@ class ContentsManager(LoggingConfigurable):
             )
         return model
 
-    def new(self, model=None, path='', ext='.ipynb'):
-        """Create a new file or directory and return its model with no content."""
+    def new(self, model=None, path='', ext=''):
+        """Create a new file or directory and return its model with no content.
+        
+        If path is a directory, a new untitled file/directory is created in path.
+        Otherwise, a new file/directory is created exactly at path.
+        """
         path = path.strip('/')
         if model is None:
             model = {}
         else:
             model.pop('path', None)
-        if 'content' not in model and model.get('type', None) != 'directory':
-            if ext == '.ipynb':
+        
+        if ext and ext != '.ipynb':
+            model.setdefault('type', 'file')
+        else:
+            model.setdefault('type', 'notebook')
+        
+        # no content, not a directory, so fill out new-file model
+        if 'content' not in model and model['type'] != 'directory':
+            if model['type'] == 'notebook':
+                ext = '.ipynb'
                 model['content'] = new_notebook()
-                model['type'] = 'notebook'
                 model['format'] = 'json'
             else:
                 model['content'] = ''
                 model['type'] = 'file'
                 model['format'] = 'text'
+        
+        # if path is a directory, create an untitled file or directory
         if self.dir_exists(path):
             if model['type'] == 'directory':
                 untitled = self.untitled_directory
@@ -252,10 +269,10 @@ class ContentsManager(LoggingConfigurable):
     def copy(self, from_path, to_path=None):
         """Copy an existing file and return its new model.
 
-        If to_name not specified, increment `from_name-Copy#.ext`.
+        If to_path not specified, it will be the parent directory of from_path.
+        If to_path is a directory, filename will increment `from_path-Copy#.ext`.
 
-        copy_from can be a full path to a file,
-        or just a base name. If a base name, `path` is used.
+        from_path must be a full path to a file.
         """
         path = from_path.strip('/')
         if '/' in path:
@@ -325,7 +342,7 @@ class ContentsManager(LoggingConfigurable):
         nb : dict
             The notebook object (in current nbformat)
         path : string
-            The notebook's directory (for logging)
+            The notebook's path (for logging)
         """
         trusted = self.notary.check_signature(nb)
         if not trusted:

IPython/html/services/contents/tests/test_contents_api.py

@@ -55,6 +55,9 @@ class API(object):
             body = json.dumps({'ext': ext})
         return self._req('POST', path, body)
 
+    def mkdir_untitled(self, path='/'):
+        return self._req('POST', path, json.dumps({'type': 'directory'}))
+
     def copy(self, copy_from, path='/'):
         body = json.dumps({'copy_from':copy_from})
         return self._req('POST', path, body)
@@ -65,6 +68,9 @@ class API(object):
     def upload(self, path, body):
         return self._req('PUT', path, body)
 
+    def mkdir_untitled(self, path='/'):
+        return self._req('POST', path, json.dumps({'type': 'directory'}))
+
     def mkdir(self, path='/'):
         return self._req('PUT', path, json.dumps({'type': 'directory'}))
 
@@ -193,8 +199,11 @@ class APITest(NotebookTestBase):
         self.assertEqual(nbnames, expected)
 
     def test_list_dirs(self):
+        print(self.api.list().json())
         dirs = dirs_only(self.api.list().json())
         dir_names = {normalize('NFC', d['name']) for d in dirs}
+        print(dir_names)
+        print(self.top_level_dirs)
         self.assertEqual(dir_names, self.top_level_dirs)  # Excluding hidden dirs
 
     def test_list_nonexistant_dir(self):
@@ -293,6 +302,18 @@ class APITest(NotebookTestBase):
         resp = self.api.upload(path, body=json.dumps(nbmodel))
         self._check_created(resp, path)
 
+    def test_mkdir_untitled(self):
+        resp = self.api.mkdir_untitled(path=u'å b')
+        self._check_created(resp, u'å b/Untitled Folder0', type='directory')
+
+        # Second time
+        resp = self.api.mkdir_untitled(path=u'å b')
+        self._check_created(resp, u'å b/Untitled Folder1', type='directory')
+
+        # And two directories down
+        resp = self.api.mkdir_untitled(path='foo/bar')
+        self._check_created(resp, 'foo/bar/Untitled Folder0', type='directory')
+
     def test_mkdir(self):
         path = u'å b/New ∂ir'
         resp = self.api.mkdir(path)

IPython/html/static/notebook/js/menubar.js

@@ -90,9 +90,9 @@ define([
         this.element.find('#new_notebook').click(function () {
             // Create a new notebook in the same path as the current
             // notebook's path.
-            var parent = utils.url_path_split(this.notebook_path)[0];
+            var parent = utils.url_path_split(that.notebook.notebook_path)[0];
             that.contents.new(parent, {
-                    ext: ".ipynb",
+                    type: "notebook",
                     extra_settings: {async: false},  // So we can open a new window afterwards
                     success: function (data) {
                         window.open(

IPython/html/static/services/contents.js

@@ -98,9 +98,13 @@ define([
      * @param {String} path The path to create the new notebook at
      * @param {Object} options:
      *      ext: file extension to use
+     *      type: model type to create ('notebook', 'file', or 'directory')
      */
     Contents.prototype.new = function(path, options) {
-        var data = JSON.stringify({ext: options.ext || ".ipynb"});
+        var data = JSON.stringify({
+          ext: options.ext,
+          type: options.type
+        });
 
         var settings = {
             processData : false,

IPython/html/static/tree/js/main.js

@@ -65,7 +65,7 @@ require([
 
     $('#new_notebook').click(function (e) {
         contents.new(common_options.notebook_path, {
-                ext: ".ipynb",
+                type: "notebook",
                 extra_settings: {async: false},  // So we can open a new window afterwards
                 success: function (data) {
                     window.open(

IPython/html/tree/handlers.py

@@ -37,9 +37,12 @@ class TreeHandler(IPythonHandler):
         path = path.strip('/')
         cm = self.contents_manager
         if cm.file_exists(path):
-            # is a notebook, redirect to notebook handler
+            # it's not a directory, we have redirecting to do
+            model = cm.get(path, content=False)
+            # redirect to /api/notebooks if it's a notebook, otherwise /api/files
+            service = 'notebooks' if model['type'] == 'notebook' else 'files'
             url = url_escape(url_path_join(
-                self.base_url, 'notebooks', path,
+                self.base_url, service, path,
             ))
             self.log.debug("Redirecting %s to %s", self.request.path, url)
             self.redirect(url)