Merge pull request #4925 from khinsen/notebook-manager-api

Notebook manager API fixes

remove various `_model` suffixes

IPython/html/services/notebooks/filenbmanager.py

@@ -82,16 +82,6 @@ class FileNotebookManager(NotebookManager):
                  for name in names]
         return names
 
-    def increment_filename(self, basename, path='', ext='.ipynb'):
-        """Return a non-used filename of the form basename<int>."""
-        path = path.strip('/')
-        for i in itertools.count():
-            name = u'{basename}{i}{ext}'.format(basename=basename, i=i, ext=ext)
-            os_path = self.get_os_path(name, path)
-            if not os.path.isfile(os_path):
-                break
-        return name
-
     def path_exists(self, path):
         """Does the API-style path (directory) actually exist?
         
@@ -231,11 +221,11 @@ class FileNotebookManager(NotebookManager):
         """
         path = path.strip('/')
         notebook_names = self.get_notebook_names(path)
-        notebooks = [self.get_notebook_model(name, path, content=False) for name in notebook_names]
+        notebooks = [self.get_notebook(name, path, content=False) for name in notebook_names]
         notebooks = sorted(notebooks, key=lambda item: item['name'])
         return notebooks
 
-    def get_notebook_model(self, name, path='', content=True):
+    def get_notebook(self, name, path='', content=True):
         """ Takes a path and name for a notebook and returns its model
         
         Parameters
@@ -276,7 +266,7 @@ class FileNotebookManager(NotebookManager):
             model['content'] = nb
         return model
 
-    def save_notebook_model(self, model, name='', path=''):
+    def save_notebook(self, model, name='', path=''):
         """Save the notebook model and return the model with no content."""
         path = path.strip('/')
 
@@ -318,20 +308,20 @@ class FileNotebookManager(NotebookManager):
             except Exception as e:
                 raise web.HTTPError(400, u'Unexpected error while saving notebook as script: %s %s' % (py_path, e))
 
-        model = self.get_notebook_model(new_name, new_path, content=False)
+        model = self.get_notebook(new_name, new_path, content=False)
         return model
 
-    def update_notebook_model(self, model, name, path=''):
+    def update_notebook(self, model, name, path=''):
         """Update the notebook's path and/or name"""
         path = path.strip('/')
         new_name = model.get('name', name)
         new_path = model.get('path', path).strip('/')
         if path != new_path or name != new_name:
             self.rename_notebook(name, path, new_name, new_path)
-        model = self.get_notebook_model(new_name, new_path, content=False)
+        model = self.get_notebook(new_name, new_path, content=False)
         return model
 
-    def delete_notebook_model(self, name, path=''):
+    def delete_notebook(self, name, path=''):
         """Delete notebook by name and path."""
         path = path.strip('/')
         os_path = self.get_os_path(name, path)

IPython/html/services/notebooks/handlers.py

@@ -84,7 +84,7 @@ class NotebookHandler(IPythonHandler):
             self.finish(json.dumps(notebooks, default=date_default))
             return
         # get and return notebook representation
-        model = nbm.get_notebook_model(name, path)
+        model = nbm.get_notebook(name, path)
         self._finish_model(model, location=False)
 
     @web.authenticated
@@ -97,7 +97,7 @@ class NotebookHandler(IPythonHandler):
         model = self.get_json_body()
         if model is None:
             raise web.HTTPError(400, u'JSON body missing')
-        model = nbm.update_notebook_model(model, name, path)
+        model = nbm.update_notebook(model, name, path)
         self._finish_model(model)
     
     def _copy_notebook(self, copy_from, path, copy_to=None):
@@ -122,7 +122,7 @@ class NotebookHandler(IPythonHandler):
         if name:
             model['name'] = name
         
-        model = self.notebook_manager.create_notebook_model(model, path)
+        model = self.notebook_manager.create_notebook(model, path)
         self.set_status(201)
         self._finish_model(model)
     
@@ -135,14 +135,14 @@ class NotebookHandler(IPythonHandler):
         model = {}
         if name:
             model['name'] = name
-        model = self.notebook_manager.create_notebook_model(model, path=path)
+        model = self.notebook_manager.create_notebook(model, path=path)
         self.set_status(201)
         self._finish_model(model)
     
     def _save_notebook(self, model, path, name):
         """Save an existing notebook."""
         self.log.info(u"Saving notebook at %s/%s", path, name)
-        model = self.notebook_manager.save_notebook_model(model, name, path)
+        model = self.notebook_manager.save_notebook(model, name, path)
         if model['path'] != path.strip('/') or model['name'] != name:
             # a rename happened, set Location header
             location = True
@@ -217,7 +217,7 @@ class NotebookHandler(IPythonHandler):
     def delete(self, path='', name=None):
         """delete the notebook in the given notebook path"""
         nbm = self.notebook_manager
-        nbm.delete_notebook_model(name, path)
+        nbm.delete_notebook(name, path)
         self.set_status(204)
         self.finish()
 

IPython/html/services/notebooks/nbmanager.py

@@ -17,6 +17,7 @@ Authors:
 # Imports
 #-----------------------------------------------------------------------------
 
+import itertools
 import os
 
 from IPython.config.configurable import LoggingConfigurable
@@ -46,26 +47,9 @@ class NotebookManager(LoggingConfigurable):
     def _notary_default(self):
         return sign.NotebookNotary(parent=self)
     
-    def check_and_sign(self, nb, path, name):
-        """Check for trusted cells, and sign the notebook.
-        
-        Called as a part of saving notebooks.
-        """
-        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, path, name):
-        """Mark cells as trusted if the notebook signature matches.
-        
-        Called as a part of loading notebooks.
-        """
-        trusted = self.notary.check_signature(nb)
-        if not trusted:
-            self.log.warn("Notebook %s/%s is not trusted", path, name)
-        self.notary.mark_cells(nb, trusted)
-    
+    # NotebookManager API part 1: methods that must be
+    # implemented in subclasses.
+
     def path_exists(self, path):
         """Does the API-style path (directory) actually exist?
         
@@ -100,34 +84,21 @@ class NotebookManager(LoggingConfigurable):
         """
         raise NotImplementedError
 
-    def _notebook_dir_changed(self, name, old, new):
-        """Do a bit of validation of the notebook dir."""
-        if not os.path.isabs(new):
-            # If we receive a non-absolute path, make it absolute.
-            self.notebook_dir = os.path.abspath(new)
-            return
-        if os.path.exists(new) and not os.path.isdir(new):
-            raise TraitError("notebook dir %r is not a directory" % new)
-        if not os.path.exists(new):
-            self.log.info("Creating notebook dir %s", new)
-            try:
-                os.mkdir(new)
-            except:
-                raise TraitError("Couldn't create notebook dir %r" % new)
-
-    # Main notebook API
+    def notebook_exists(self, name, path=''):
+        """Returns a True if the notebook exists. Else, returns False.
 
-    def increment_filename(self, basename, path=''):
-        """Increment a notebook filename without the .ipynb to make it unique.
-        
         Parameters
         ----------
-        basename : unicode
-            The name of a notebook without the ``.ipynb`` file extension.
-        path : unicode
-            The URL path of the notebooks directory
+        name : string
+            The name of the notebook you are checking.
+        path : string
+            The relative path to the notebook (with '/' as separator)
+
+        Returns
+        -------
+        bool
         """
-        return basename
+        raise NotImplementedError('must be implemented in a subclass')
 
     # TODO: Remove this after we create the contents web service and directories are
     # no longer listed by the notebook web service.
@@ -162,23 +133,72 @@ class NotebookManager(LoggingConfigurable):
         """
         raise NotImplementedError('must be implemented in a subclass')
 
-    def get_notebook_model(self, name, path='', content=True):
+    def get_notebook(self, name, path='', content=True):
         """Get the notebook model with or without content."""
         raise NotImplementedError('must be implemented in a subclass')
 
-    def save_notebook_model(self, model, name, path=''):
-        """Save the notebook model and return the model with no content."""
+    def save_notebook(self, model, name, path=''):
+        """Save the notebook and return the model with no content."""
         raise NotImplementedError('must be implemented in a subclass')
 
-    def update_notebook_model(self, model, name, path=''):
-        """Update the notebook model and return the model with no content."""
+    def update_notebook(self, model, name, path=''):
+        """Update the notebook and return the model with no content."""
         raise NotImplementedError('must be implemented in a subclass')
 
-    def delete_notebook_model(self, name, path=''):
+    def delete_notebook(self, name, path=''):
         """Delete notebook by name and path."""
         raise NotImplementedError('must be implemented in a subclass')
 
-    def create_notebook_model(self, model=None, path=''):
+    def create_checkpoint(self, name, path=''):
+        """Create a checkpoint of the current state of a notebook
+        
+        Returns a checkpoint_id for the new checkpoint.
+        """
+        raise NotImplementedError("must be implemented in a subclass")
+    
+    def list_checkpoints(self, name, path=''):
+        """Return a list of checkpoints for a given notebook"""
+        return []
+    
+    def restore_checkpoint(self, checkpoint_id, name, path=''):
+        """Restore a notebook from one of its checkpoints"""
+        raise NotImplementedError("must be implemented in a subclass")
+
+    def delete_checkpoint(self, checkpoint_id, name, path=''):
+        """delete a checkpoint for a notebook"""
+        raise NotImplementedError("must be implemented in a subclass")
+    
+    def info_string(self):
+        return "Serving notebooks"
+
+    # NotebookManager API part 2: methods that have useable default
+    # implementations, but can be overridden in subclasses.
+
+    def increment_filename(self, basename, path=''):
+        """Increment a notebook filename without the .ipynb to make it unique.
+        
+        Parameters
+        ----------
+        basename : unicode
+            The name of a notebook without the ``.ipynb`` file extension.
+        path : unicode
+            The URL path of the notebooks directory
+
+        Returns
+        -------
+        name : unicode
+            A notebook name (with the .ipynb extension) that starts
+            with basename and does not refer to any existing notebook.
+        """
+        path = path.strip('/')
+        for i in itertools.count():
+            name = u'{basename}{i}{ext}'.format(basename=basename, i=i,
+                                                ext=self.filename_ext)
+            if not self.notebook_exists(name, path):
+                break
+        return name
+
+    def create_notebook(self, model=None, path=''):
         """Create a new notebook and return its model with no content."""
         path = path.strip('/')
         if model is None:
@@ -190,7 +210,7 @@ class NotebookManager(LoggingConfigurable):
             model['name'] = self.increment_filename('Untitled', path)
             
         model['path'] = path
-        model = self.save_notebook_model(model, model['name'], model['path'])
+        model = self.save_notebook(model, model['name'], model['path'])
         return model
 
     def copy_notebook(self, from_name, to_name=None, path=''):
@@ -199,37 +219,51 @@ class NotebookManager(LoggingConfigurable):
         If to_name not specified, increment `from_name-Copy#.ipynb`.
         """
         path = path.strip('/')
-        model = self.get_notebook_model(from_name, path)
+        model = self.get_notebook(from_name, path)
         if not to_name:
             base = os.path.splitext(from_name)[0] + '-Copy'
             to_name = self.increment_filename(base, path)
         model['name'] = to_name
-        model = self.save_notebook_model(model, to_name, path)
+        model = self.save_notebook(model, to_name, path)
         return model
     
-    # Checkpoint-related
-    
-    def create_checkpoint(self, name, path=''):
-        """Create a checkpoint of the current state of a notebook
+    def log_info(self):
+        self.log.info(self.info_string())
+
+    # NotebookManager methods provided for use in subclasses.
+
+    def check_and_sign(self, nb, path, name):
+        """Check for trusted cells, and sign the notebook.
         
-        Returns a checkpoint_id for the new checkpoint.
+        Called as a part of saving notebooks.
         """
-        raise NotImplementedError("must be implemented in a subclass")
+        if self.notary.check_cells(nb):
+            self.notary.sign(nb)
+        else:
+            self.log.warn("Saving untrusted notebook %s/%s", path, name)
     
-    def list_checkpoints(self, name, path=''):
-        """Return a list of checkpoints for a given notebook"""
-        return []
+    def mark_trusted_cells(self, nb, path, name):
+        """Mark cells as trusted if the notebook signature matches.
+        
+        Called as a part of loading notebooks.
+        """
+        trusted = self.notary.check_signature(nb)
+        if not trusted:
+            self.log.warn("Notebook %s/%s is not trusted", path, name)
+        self.notary.mark_cells(nb, trusted)
     
-    def restore_checkpoint(self, checkpoint_id, name, path=''):
-        """Restore a notebook from one of its checkpoints"""
-        raise NotImplementedError("must be implemented in a subclass")
+    def _notebook_dir_changed(self, name, old, new):
+        """Do a bit of validation of the notebook dir."""
+        if not os.path.isabs(new):
+            # If we receive a non-absolute path, make it absolute.
+            self.notebook_dir = os.path.abspath(new)
+            return
+        if os.path.exists(new) and not os.path.isdir(new):
+            raise TraitError("notebook dir %r is not a directory" % new)
+        if not os.path.exists(new):
+            self.log.info("Creating notebook dir %s", new)
+            try:
+                os.mkdir(new)
+            except:
+                raise TraitError("Couldn't create notebook dir %r" % new)
 
-    def delete_checkpoint(self, checkpoint_id, name, path=''):
-        """delete a checkpoint for a notebook"""
-        raise NotImplementedError("must be implemented in a subclass")
-    
-    def log_info(self):
-        self.log.info(self.info_string())
-    
-    def info_string(self):
-        return "Serving notebooks"

IPython/html/services/notebooks/tests/test_nbmanager.py

@@ -70,11 +70,11 @@ class TestNotebookManager(TestCase):
         except OSError:
             print("Directory already exists: %r" % os_path)
 
-    def test_create_notebook_model(self):
+    def test_create_notebook(self):
         with TemporaryDirectory() as td:
             # Test in root directory
             nm = FileNotebookManager(notebook_dir=td)
-            model = nm.create_notebook_model()
+            model = nm.create_notebook()
             assert isinstance(model, dict)
             self.assertIn('name', model)
             self.assertIn('path', model)
@@ -84,24 +84,24 @@ class TestNotebookManager(TestCase):
             # Test in sub-directory
             sub_dir = '/foo/'
             self.make_dir(nm.notebook_dir, 'foo')
-            model = nm.create_notebook_model(None, sub_dir)
+            model = nm.create_notebook(None, sub_dir)
             assert isinstance(model, dict)
             self.assertIn('name', model)
             self.assertIn('path', model)
             self.assertEqual(model['name'], 'Untitled0.ipynb')
             self.assertEqual(model['path'], sub_dir.strip('/'))
 
-    def test_get_notebook_model(self):
+    def test_get_notebook(self):
         with TemporaryDirectory() as td:
             # Test in root directory
             # Create a notebook
             nm = FileNotebookManager(notebook_dir=td)
-            model = nm.create_notebook_model()
+            model = nm.create_notebook()
             name = model['name']
             path = model['path']
 
             # Check that we 'get' on the notebook we just created
-            model2 = nm.get_notebook_model(name, path)
+            model2 = nm.get_notebook(name, path)
             assert isinstance(model2, dict)
             self.assertIn('name', model2)
             self.assertIn('path', model2)
@@ -111,8 +111,8 @@ class TestNotebookManager(TestCase):
             # Test in sub-directory
             sub_dir = '/foo/'
             self.make_dir(nm.notebook_dir, 'foo')
-            model = nm.create_notebook_model(None, sub_dir)
-            model2 = nm.get_notebook_model(name, sub_dir)
+            model = nm.create_notebook(None, sub_dir)
+            model2 = nm.get_notebook(name, sub_dir)
             assert isinstance(model2, dict)
             self.assertIn('name', model2)
             self.assertIn('path', model2)
@@ -120,37 +120,37 @@ class TestNotebookManager(TestCase):
             self.assertEqual(model2['name'], 'Untitled0.ipynb')
             self.assertEqual(model2['path'], sub_dir.strip('/'))
             
-    def test_update_notebook_model(self):
+    def test_update_notebook(self):
         with TemporaryDirectory() as td:
             # Test in root directory
             # Create a notebook
             nm = FileNotebookManager(notebook_dir=td)
-            model = nm.create_notebook_model()
+            model = nm.create_notebook()
             name = model['name']
             path = model['path']
 
             # Change the name in the model for rename
             model['name'] = 'test.ipynb'
-            model = nm.update_notebook_model(model, name, path)
+            model = nm.update_notebook(model, name, path)
             assert isinstance(model, dict)
             self.assertIn('name', model)
             self.assertIn('path', model)
             self.assertEqual(model['name'], 'test.ipynb')
 
             # Make sure the old name is gone
-            self.assertRaises(HTTPError, nm.get_notebook_model, name, path)
+            self.assertRaises(HTTPError, nm.get_notebook, name, path)
 
             # Test in sub-directory
             # Create a directory and notebook in that directory
             sub_dir = '/foo/'
             self.make_dir(nm.notebook_dir, 'foo')
-            model = nm.create_notebook_model(None, sub_dir)
+            model = nm.create_notebook(None, sub_dir)
             name = model['name']
             path = model['path']
             
             # Change the name in the model for rename
             model['name'] = 'test_in_sub.ipynb'
-            model = nm.update_notebook_model(model, name, path)
+            model = nm.update_notebook(model, name, path)
             assert isinstance(model, dict)
             self.assertIn('name', model)
             self.assertIn('path', model)
@@ -158,22 +158,22 @@ class TestNotebookManager(TestCase):
             self.assertEqual(model['path'], sub_dir.strip('/'))
             
             # Make sure the old name is gone
-            self.assertRaises(HTTPError, nm.get_notebook_model, name, path)
+            self.assertRaises(HTTPError, nm.get_notebook, name, path)
 
-    def test_save_notebook_model(self):
+    def test_save_notebook(self):
         with TemporaryDirectory() as td:
             # Test in the root directory
             # Create a notebook
             nm = FileNotebookManager(notebook_dir=td)
-            model = nm.create_notebook_model()
+            model = nm.create_notebook()
             name = model['name']
             path = model['path']
 
             # Get the model with 'content'
-            full_model = nm.get_notebook_model(name, path)
+            full_model = nm.get_notebook(name, path)
 
             # Save the notebook
-            model = nm.save_notebook_model(full_model, name, path)
+            model = nm.save_notebook(full_model, name, path)
             assert isinstance(model, dict)
             self.assertIn('name', model)
             self.assertIn('path', model)
@@ -184,13 +184,13 @@ class TestNotebookManager(TestCase):
             # Create a directory and notebook in that directory
             sub_dir = '/foo/'
             self.make_dir(nm.notebook_dir, 'foo')
-            model = nm.create_notebook_model(None, sub_dir)
+            model = nm.create_notebook(None, sub_dir)
             name = model['name']
             path = model['path']
-            model = nm.get_notebook_model(name, path)
+            model = nm.get_notebook(name, path)
 
             # Change the name in the model for rename
-            model = nm.save_notebook_model(model, name, path)
+            model = nm.save_notebook(model, name, path)
             assert isinstance(model, dict)
             self.assertIn('name', model)
             self.assertIn('path', model)
@@ -202,34 +202,34 @@ class TestNotebookManager(TestCase):
             # Create a notebook
             nm = FileNotebookManager(notebook_dir=td)
             nm.save_script = True
-            model = nm.create_notebook_model()
+            model = nm.create_notebook()
             name = model['name']
             path = model['path']
 
             # Get the model with 'content'
-            full_model = nm.get_notebook_model(name, path)
+            full_model = nm.get_notebook(name, path)
 
             # Save the notebook
-            model = nm.save_notebook_model(full_model, name, path)
+            model = nm.save_notebook(full_model, name, path)
 
             # Check that the script was created
             py_path = os.path.join(td, os.path.splitext(name)[0]+'.py')
             assert os.path.exists(py_path), py_path
 
-    def test_delete_notebook_model(self):
+    def test_delete_notebook(self):
         with TemporaryDirectory() as td:
             # Test in the root directory
             # Create a notebook
             nm = FileNotebookManager(notebook_dir=td)
-            model = nm.create_notebook_model()
+            model = nm.create_notebook()
             name = model['name']
             path = model['path']
             
             # Delete the notebook
-            nm.delete_notebook_model(name, path)
+            nm.delete_notebook(name, path)
             
             # Check that a 'get' on the deleted notebook raises and error
-            self.assertRaises(HTTPError, nm.get_notebook_model, name, path)
+            self.assertRaises(HTTPError, nm.get_notebook, name, path)
     
     def test_copy_notebook(self):
         with TemporaryDirectory() as td:
@@ -239,7 +239,7 @@ class TestNotebookManager(TestCase):
             path = u'å b'
             name = u'nb √.ipynb'
             os.mkdir(os.path.join(td, path))
-            orig = nm.create_notebook_model({'name' : name}, path=path)
+            orig = nm.create_notebook({'name' : name}, path=path)
             
             # copy with unspecified name
             copy = nm.copy_notebook(name, path=path)