From 3c26b079f03b9daa36df09def706318846b2e495 Mon Sep 17 00:00:00 2001 From: MinRK Date: Tue, 22 Jul 2014 14:39:54 -0700 Subject: [PATCH] updates per review - clarified docstrings and errors - still more notebook/file renames - configurable untitled names - copy_from can be full path - fix running, upload, new-tab behaviors in dashboard Yay, review! --- IPython/html/services/contents/filemanager.py | 27 ++-- IPython/html/services/contents/handlers.py | 38 +++--- IPython/html/services/contents/manager.py | 120 ++++++++++++++---- .../contents/tests/test_contents_api.py | 10 +- IPython/html/static/tree/js/kernellist.js | 21 ++- IPython/html/static/tree/js/notebooklist.js | 14 +- 6 files changed, 168 insertions(+), 62 deletions(-) diff --git a/IPython/html/services/contents/filemanager.py b/IPython/html/services/contents/filemanager.py index a2f35eea5..3f56baa5c 100644 --- a/IPython/html/services/contents/filemanager.py +++ b/IPython/html/services/contents/filemanager.py @@ -54,7 +54,7 @@ class FileContentsManager(ContentsManager): self.log.debug("copystat on %s failed", dest, exc_info=True) def _get_os_path(self, name=None, path=''): - """Given a filename and a URL path, return its file system + """Given a filename and API path, return its file system path. Parameters @@ -62,8 +62,7 @@ class FileContentsManager(ContentsManager): name : string A filename path : string - The relative URL path (with '/' as separator) to the named - file. + The relative API path to the named file. Returns ------- @@ -77,6 +76,8 @@ class FileContentsManager(ContentsManager): def path_exists(self, path): """Does the API-style path refer to an extant directory? + API-style wrapper for os.path.isdir + Parameters ---------- path : string @@ -114,6 +115,8 @@ class FileContentsManager(ContentsManager): def file_exists(self, name, path=''): """Returns True if the file exists, else returns False. + API-style wrapper for os.path.isfile + Parameters ---------- name : string @@ -123,7 +126,8 @@ class FileContentsManager(ContentsManager): Returns ------- - bool + exists : bool + Whether the file exists. """ path = path.strip('/') nbpath = self._get_os_path(name, path=path) @@ -132,6 +136,8 @@ class FileContentsManager(ContentsManager): def exists(self, name=None, path=''): """Returns True if the path [and name] exists, else returns False. + API-style wrapper for os.path.exists + Parameters ---------- name : string @@ -141,7 +147,8 @@ class FileContentsManager(ContentsManager): Returns ------- - bool + exists : bool + Whether the target exists. """ path = path.strip('/') os_path = self._get_os_path(name, path=path) @@ -246,7 +253,7 @@ class FileContentsManager(ContentsManager): name : str the name of the target path : str - the URL path that describes the relative path for the target + the API path that describes the relative path for the target Returns ------- @@ -344,7 +351,11 @@ class FileContentsManager(ContentsManager): return model def update(self, model, name, path=''): - """Update the file's path and/or name""" + """Update the file's path and/or name + + For use in PATCH requests, to enable renaming a file without + re-uploading its contents. Only used for renaming at the moment. + """ path = path.strip('/') new_name = model.get('name', name) new_path = model.get('path', path).strip('/') @@ -393,7 +404,7 @@ class FileContentsManager(ContentsManager): # Should we proceed with the move? if os.path.isfile(new_os_path): - raise web.HTTPError(409, u'Notebook with name already exists: %s' % new_os_path) + raise web.HTTPError(409, u'File with name already exists: %s' % new_os_path) # Move the file try: diff --git a/IPython/html/services/contents/handlers.py b/IPython/html/services/contents/handlers.py index 7f394f3ba..72860ad67 100644 --- a/IPython/html/services/contents/handlers.py +++ b/IPython/html/services/contents/handlers.py @@ -54,16 +54,16 @@ class ContentsHandler(IPythonHandler): @web.authenticated @json_errors def get(self, path='', name=None): - """Return a file or list of files. + """Return a model for a file or directory. - * GET with path and no filename lists files in a directory - * GET with path and filename returns file contents model + A directory model contains a list of models (without content) + of the files and directories it contains. """ path = path or '' model = self.contents_manager.get_model(name=name, path=path) if model['type'] == 'directory': # group listing by type, then by name (case-insensitive) - # FIXME: front-ends shouldn't rely on this sorting + # FIXME: sorting should be done in the frontends model['content'].sort(key=sort_key) self._finish_model(model, location=False) @@ -81,22 +81,22 @@ class ContentsHandler(IPythonHandler): self._finish_model(model) def _copy(self, copy_from, path, copy_to=None): - """Copy a file in path, optionally specifying the new name. - - Only support copying within the same directory. + """Copy a file, optionally specifying the new name. """ - self.log.info(u"Copying from %s/%s to %s/%s", - path, copy_from, - path, copy_to or '', - ) + self.log.info(u"Copying {copy_from} to {path}/{copy_to}".format( + copy_from=copy_from, + path=path, + copy_to=copy_to or '', + )) model = self.contents_manager.copy(copy_from, copy_to, path) self.set_status(201) self._finish_model(model) def _upload(self, model, path, name=None): - """Upload a file + """Handle upload of a new file - If name specified, create it in path/name. + If name specified, create it in path/name, + otherwise create a new untitled file in path. """ self.log.info(u"Uploading file to %s/%s", path, name or '') if name: @@ -151,7 +151,7 @@ class ContentsHandler(IPythonHandler): cm = self.contents_manager if cm.file_exists(path): - raise web.HTTPError(400, "Only POST to directories. Use PUT for full names.") + raise web.HTTPError(400, "Cannot POST to existing files, use PUT instead.") if not cm.path_exists(path): raise web.HTTPError(404, "No such directory: %s" % path) @@ -184,11 +184,17 @@ class ContentsHandler(IPythonHandler): Save notebook at ``path/Name.ipynb``. Notebook structure is specified in `content` key of JSON request body. If content is not specified, create a new empty notebook. - PUT /api/contents/path/Name.ipynb?copy=OtherNotebook.ipynb + PUT /api/contents/path/Name.ipynb + with JSON body:: + + { + "copy_from" : "[path/to/]OtherNotebook.ipynb" + } + Copy OtherNotebook to Name """ if name is None: - raise web.HTTPError(400, "Only PUT to full names. Use POST for directories.") + raise web.HTTPError(400, "name must be specified with PUT.") model = self.get_json_body() if model: diff --git a/IPython/html/services/contents/manager.py b/IPython/html/services/contents/manager.py index 2a8c7760b..e6a11ed4d 100644 --- a/IPython/html/services/contents/manager.py +++ b/IPython/html/services/contents/manager.py @@ -15,6 +15,31 @@ from IPython.utils.traitlets import Instance, Unicode, List class ContentsManager(LoggingConfigurable): + """Base class for serving files and directories. + + This serves any text or binary file, + as well as directories, + with special handling for JSON notebook documents. + + Most APIs take a path argument, + which is always an API-style unicode path, + and always refers to a directory. + + - unicode, not url-escaped + - '/'-separated + - leading and trailing '/' will be stripped + - if unspecified, path defaults to '', + indicating the root path. + + name is also unicode, and refers to a specfic target: + + - unicode, not url-escaped + - must not contain '/' + - It refers to an individual filename + - It may refer to a directory name, + in the case of listing or creating directories. + + """ notary = Instance(sign.NotebookNotary) def _notary_default(self): @@ -27,12 +52,26 @@ class ContentsManager(LoggingConfigurable): Glob patterns to hide in file and directory listings. """) + untitled_notebook = Unicode("Untitled", config=True, + help="The base name used when creating untitled notebooks." + ) + + untitled_file = Unicode("untitled", config=True, + help="The base name used when creating untitled files." + ) + + untitled_directory = Unicode("Untitled Folder", config=True, + help="The base name used when creating untitled directories." + ) + # ContentsManager API part 1: methods that must be # implemented in subclasses. def path_exists(self, path): """Does the API-style path (directory) actually exist? + Like os.path.isdir + Override this method in subclasses. Parameters @@ -58,14 +97,18 @@ class ContentsManager(LoggingConfigurable): Returns ------- - exists : bool + hidden : bool Whether the path is hidden. """ raise NotImplementedError def file_exists(self, name, path=''): - """Returns a True if the file exists. Else, returns False. + """Does a file exist at the given name and path? + + Like os.path.isfile + + Override this method in subclasses. Parameters ---------- @@ -76,20 +119,29 @@ class ContentsManager(LoggingConfigurable): Returns ------- - bool + exists : bool + Whether the file exists. """ raise NotImplementedError('must be implemented in a subclass') - def list(self, path=''): - """Return a list of contents dicts without content. + def exists(self, name, path=''): + """Does a file or directory exist at the given name and path? - This returns a list of dicts + Like os.path.exists - This list of dicts should be sorted by name:: + Parameters + ---------- + name : string + The name of the file you are checking. + path : string + The relative path to the file's directory (with '/' as separator) - data = sorted(data, key=lambda item: item['name']) + Returns + ------- + exists : bool + Whether the target exists. """ - raise NotImplementedError('must be implemented in a subclass') + return self.file_exists(name, path) or self.path_exists("%s/%s" % (path, name)) def get_model(self, name, path='', content=True): """Get the model of a file or directory with or without content.""" @@ -100,7 +152,11 @@ class ContentsManager(LoggingConfigurable): raise NotImplementedError('must be implemented in a subclass') def update(self, model, name, path=''): - """Update the file or directory and return the model with no content.""" + """Update the file or directory and return the model with no content. + + For use in PATCH requests, to enable renaming a file without + re-uploading its contents. Only used for renaming at the moment. + """ raise NotImplementedError('must be implemented in a subclass') def delete(self, name, path=''): @@ -126,12 +182,12 @@ class ContentsManager(LoggingConfigurable): """delete a checkpoint for a file""" raise NotImplementedError("must be implemented in a subclass") - def info_string(self): - return "Serving notebooks" - # ContentsManager API part 2: methods that have useable default # implementations, but can be overridden in subclasses. + def info_string(self): + return "Serving contents" + def get_kernel_path(self, name, path='', model=None): """ Return the path to start kernel in """ return path @@ -144,7 +200,7 @@ class ContentsManager(LoggingConfigurable): filename : unicode The name of a file, including extension path : unicode - The URL path of the target's directory + The API path of the target's directory Returns ------- @@ -176,7 +232,15 @@ class ContentsManager(LoggingConfigurable): model['type'] = 'file' model['format'] = 'text' if 'name' not in model: - model['name'] = self.increment_filename('Untitled' + ext, path) + if model['type'] == 'directory': + untitled = self.untitled_directory + elif model['type'] == 'notebook': + untitled = self.untitled_notebook + elif model['type'] == 'file': + untitled = self.untitled_file + else: + raise HTTPError(400, "Unexpected model type: %r" % model['type']) + model['name'] = self.increment_filename(untitled + ext, path) model['path'] = path model = self.save(model, model['name'], model['path']) @@ -186,9 +250,16 @@ class ContentsManager(LoggingConfigurable): """Copy an existing file and return its new model. If to_name not specified, increment `from_name-Copy#.ext`. + + copy_from can be a full path to a file, + or just a base name. If a base name, `path` is used. """ path = path.strip('/') - model = self.get_model(from_name, path) + if '/' in from_name: + from_path, from_name = from_name.rsplit('/', 1) + else: + from_path = path + model = self.get_model(from_name, from_path) if model['type'] == 'directory': raise HTTPError(400, "Can't copy directories") if not to_name: @@ -196,6 +267,7 @@ class ContentsManager(LoggingConfigurable): copy_name = u'{0}-Copy{1}'.format(base, ext) to_name = self.increment_filename(copy_name, path) model['name'] = to_name + model['path'] = path model = self.save(model, to_name, path) return model @@ -218,7 +290,7 @@ class ContentsManager(LoggingConfigurable): self.notary.mark_cells(nb, True) self.save(model, name, path) - def check_and_sign(self, nb, name, path=''): + def check_and_sign(self, nb, name='', path=''): """Check for trusted cells, and sign the notebook. Called as a part of saving notebooks. @@ -226,18 +298,18 @@ class ContentsManager(LoggingConfigurable): Parameters ---------- nb : dict - The notebook structure + The notebook object (in nbformat.current format) name : string - The filename of the notebook + The filename of the notebook (for logging) path : string - The notebook's directory + The notebook's directory (for logging) """ if self.notary.check_cells(nb): self.notary.sign(nb) else: self.log.warn("Saving untrusted notebook %s/%s", path, name) - def mark_trusted_cells(self, nb, name, path=''): + def mark_trusted_cells(self, nb, name='', path=''): """Mark cells as trusted if the notebook signature matches. Called as a part of loading notebooks. @@ -245,11 +317,11 @@ class ContentsManager(LoggingConfigurable): Parameters ---------- nb : dict - The notebook structure + The notebook object (in nbformat.current format) name : string - The filename of the notebook + The filename of the notebook (for logging) path : string - The notebook's directory + The notebook's directory (for logging) """ trusted = self.notary.check_signature(nb) if not trusted: diff --git a/IPython/html/services/contents/tests/test_contents_api.py b/IPython/html/services/contents/tests/test_contents_api.py index aadf0c93b..12dbf6be4 100644 --- a/IPython/html/services/contents/tests/test_contents_api.py +++ b/IPython/html/services/contents/tests/test_contents_api.py @@ -22,8 +22,6 @@ from IPython.utils import py3compat from IPython.utils.data import uniq_stable -# TODO: Remove this after we create the contents web service and directories are -# no longer listed by the notebook web service. def notebooks_only(dir_model): return [nb for nb in dir_model['content'] if nb['type']=='notebook'] @@ -279,9 +277,9 @@ class APITest(NotebookTestBase): def test_create_untitled_txt(self): resp = self.api.create_untitled(path='foo/bar', ext='.txt') - self._check_created(resp, 'Untitled0.txt', 'foo/bar', type='file') + self._check_created(resp, 'untitled0.txt', 'foo/bar', type='file') - resp = self.api.read(path='foo/bar', name='Untitled0.txt') + resp = self.api.read(path='foo/bar', name='untitled0.txt') model = resp.json() self.assertEqual(model['type'], 'file') self.assertEqual(model['format'], 'text') @@ -363,6 +361,10 @@ class APITest(NotebookTestBase): resp = self.api.copy(u'ç d.ipynb', u'cøpy.ipynb', path=u'å b') self._check_created(resp, u'cøpy.ipynb', u'å b') + def test_copy_path(self): + resp = self.api.copy(u'foo/a.ipynb', u'cøpyfoo.ipynb', path=u'å b') + self._check_created(resp, u'cøpyfoo.ipynb', u'å b') + def test_copy_dir_400(self): # can't copy directories with assert_http_error(400): diff --git a/IPython/html/static/tree/js/kernellist.js b/IPython/html/static/tree/js/kernellist.js index 60b5628ef..a4c318f58 100644 --- a/IPython/html/static/tree/js/kernellist.js +++ b/IPython/html/static/tree/js/kernellist.js @@ -19,7 +19,7 @@ define([ // base_url: string // notebook_path: string notebooklist.NotebookList.call(this, selector, $.extend({ - element_name: 'running'}, + element_name: 'running'}, options)); }; @@ -28,13 +28,20 @@ define([ KernelList.prototype.sessions_loaded = function (d) { this.sessions = d; this.clear_list(); - var item; - for (var path in d) { - item = this.new_notebook_item(-1); - this.add_link('', path, item); - this.add_shutdown_button(item, this.sessions[path]); + var item, path_name; + for (path_name in d) { + if (!d.hasOwnProperty(path_name)) { + // nothing is safe in javascript + continue; + } + item = this.new_item(-1); + this.add_link({ + name: path_name, + path: '', + type: 'notebook', + }, item); + this.add_shutdown_button(item, this.sessions[path_name]); } - $('#running_list_header').toggle($.isEmptyObject(d)); }; diff --git a/IPython/html/static/tree/js/notebooklist.js b/IPython/html/static/tree/js/notebooklist.js index c973bd817..6c9aedece 100644 --- a/IPython/html/static/tree/js/notebooklist.js +++ b/IPython/html/static/tree/js/notebooklist.js @@ -80,7 +80,7 @@ define([ var name_and_ext = utils.splitext(f.name); var file_ext = name_and_ext[1]; if (file_ext === '.ipynb') { - var item = that.new_notebook_item(0); + var item = that.new_item(0); item.addClass('new-file'); that.add_name_input(f.name, item); // Store the notebook item in the reader so we can use it later @@ -236,7 +236,7 @@ define([ var icon = NotebookList.icons[model.type]; var uri_prefix = NotebookList.uri_prefixes[model.type]; item.find(".item_icon").addClass(icon).addClass('icon-fixed-width'); - item.find("a.item_link") + var link = item.find("a.item_link") .attr('href', utils.url_join_encode( this.base_url, @@ -245,6 +245,11 @@ define([ name ) ); + // directory nav doesn't open new tabs + // files, notebooks do + if (model.type !== "directory") { + link.attr('target','_blank'); + } var path_name = utils.url_path_join(path, name); if (model.type == 'file') { this.add_delete_button(item); @@ -362,7 +367,10 @@ define([ var nbdata = item.data('nbdata'); var content_type = 'application/json'; var model = { + path: path, + name: nbname, content : JSON.parse(nbdata), + type : 'notebook' }; var settings = { processData : false, @@ -372,7 +380,7 @@ define([ data : JSON.stringify(model), headers : {'Content-Type': content_type}, success : function (data, status, xhr) { - that.add_link(path, nbname, item); + that.add_link(model, item); that.add_delete_button(item); }, error : utils.log_ajax_error,