address review from takluyver

- various typos
- discuss multi-line strings in nbformat doc
- testing cleanup
- py3compat simplification
- don't use setdefault when composing notebook nodes
- mime-type fix in svg2pdf

IPython/html/tests/notebook/dualmode_cellinsert.js

@@ -57,7 +57,7 b' casper.notebook_test(function () {'
         this.trigger_keydown('b'); // new cell below
         this.test.assertEquals(this.get_cell(3).cell_type, 'code', 'b; inserts a code cell below code cell');
         this.trigger_keydown('a'); // new cell above
-        this.test.assertEquals(this.get_cell(3).cell_type, 'code', 'a; inserts a code cell below code cell');
+        this.test.assertEquals(this.get_cell(3).cell_type, 'code', 'a; inserts a code cell above code cell');
     });
     
     this.thenEvaluate(function() {
@@ -72,6 +72,6 b' casper.notebook_test(function () {'
         this.test.assertEquals(this.get_cell(2).cell_type, 'raw', 'a; inserts a raw cell above raw cell');
         this.trigger_keydown('y'); // switch it to code for the next test
         this.trigger_keydown('b'); // new cell below
-        this.test.assertEquals(this.get_cell(3).cell_type, 'raw', 'b; inserts a raw cell above raw cell');
+        this.test.assertEquals(this.get_cell(3).cell_type, 'raw', 'b; inserts a raw cell below raw cell');
     });
 });

IPython/nbconvert/preprocessors/extractoutput.py

@@ -72,14 +72,9 b' class ExtractOutputPreprocessor(Preprocessor):'
                     else:
                         data = data.encode("UTF-8")
                     
-                    # Build an output name
-                    # filthy hack while we have some mimetype output, and some not
-                    if '/' in mime_type:
-                        ext = guess_extension(mime_type)
-                        if ext is None:
-                            ext = '.' + mime_type.rsplit('/')[-1]
-                    else:
-                        ext = '.' + mime_type
+                    ext = guess_extension(mime_type)
+                    if ext is None:
+                        ext = '.' + mime_type.rsplit('/')[-1]
                     
                     filename = self.output_filename_template.format(
                                     unique_key=unique_key,
@@ -87,10 +82,10 b' class ExtractOutputPreprocessor(Preprocessor):'
                                     index=index,
                                     extension=ext)
 
-                    #On the cell, make the figure available via 
+                    # On the cell, make the figure available via
                     #   cell.outputs[i].metadata.filenames['mime/type']
-                    # Where
-                    #   cell.outputs[i].data['mime/type' contains the data
+                    # where
+                    #   cell.outputs[i].data['mime/type'] contains the data
                     if output_files_dir is not None:
                         filename = os.path.join(output_files_dir, filename)
                     out.metadata.setdefault('filenames', {})

IPython/nbconvert/preprocessors/svg2pdf.py

@@ -1,17 +1,9 b''
 """Module containing a preprocessor that converts outputs in the notebook from 
 one format to another.
 """
-#-----------------------------------------------------------------------------
-# Copyright (c) 2013, the IPython Development Team.
-#
-# Distributed under the terms of the Modified BSD License.
-#
-# The full license is in the file COPYING.txt, distributed with this software.
-#-----------------------------------------------------------------------------
 
-#-----------------------------------------------------------------------------
-# Imports
-#-----------------------------------------------------------------------------
+# Copyright (c) IPython Development Team.
+# Distributed under the terms of the Modified BSD License.
 
 import base64
 import io
@@ -25,15 +17,8 b' from IPython.utils.traitlets import Unicode'
 from .convertfigures import ConvertFiguresPreprocessor
 
 
-#-----------------------------------------------------------------------------
-# Constants
-#-----------------------------------------------------------------------------
-
 INKSCAPE_APP = '/Applications/Inkscape.app/Contents/Resources/bin/inkscape'
 
-#-----------------------------------------------------------------------------
-# Classes
-#-----------------------------------------------------------------------------
 
 class SVG2PDFPreprocessor(ConvertFiguresPreprocessor):
     """
@@ -41,7 +26,7 b' class SVG2PDFPreprocessor(ConvertFiguresPreprocessor):'
     """
     
     def _from_format_default(self):
-        return 'svg'
+        return 'image/svg+xml'
     def _to_format_default(self):
         return 'application/pdf'
     

IPython/nbconvert/preprocessors/tests/base.py

@@ -16,16 +16,16 b' class PreprocessorTestsBase(TestsBase):'
     def build_notebook(self):
         """Build a notebook in memory for use with preprocessor tests"""
 
-        outputs = [nbformat.new_output(output_type="stream", name="stdout", text="a"),
-                   nbformat.new_output(output_type="display_data", data={'text/plain': 'b'}),
-                   nbformat.new_output(output_type="stream", name="stdout", text="c"),
-                   nbformat.new_output(output_type="stream", name="stdout", text="d"),
-                   nbformat.new_output(output_type="stream", name="stderr", text="e"),
-                   nbformat.new_output(output_type="stream", name="stderr", text="f"),
-                   nbformat.new_output(output_type="display_data", data={'image/png': 'Zw=='})] # g
-        out = nbformat.new_output(output_type="display_data")
-        out.data['application/pdf'] = 'aA=='
-        outputs.append(out)
+        outputs = [
+            nbformat.new_output("stream", name="stdout", text="a"),
+            nbformat.new_output("display_data", data={'text/plain': 'b'}),
+            nbformat.new_output("stream", name="stdout", text="c"),
+            nbformat.new_output("stream", name="stdout", text="d"),
+            nbformat.new_output("stream", name="stderr", text="e"),
+            nbformat.new_output("stream", name="stderr", text="f"),
+            nbformat.new_output("display_data", data={'image/png': 'Zw=='}), # g
+            nbformat.new_output("display_data", data={'application/pdf': 'aA=='}), # h
+        ]
         
         cells=[nbformat.new_code_cell(source="$ e $", execution_count=1, outputs=outputs),
                nbformat.new_markdown_cell(source="$ e $")]

IPython/nbconvert/preprocessors/tests/test_execute.py

@@ -47,18 +47,18 b' class TestExecute(PreprocessorTestsBase):'
     def assert_notebooks_equal(self, expected, actual):
         expected_cells = expected['cells']
         actual_cells = actual['cells']
-        assert len(expected_cells) == len(actual_cells)
+        self.assertEqual(len(expected_cells), len(actual_cells))
 
         for expected_cell, actual_cell in zip(expected_cells, actual_cells):
             expected_outputs = expected_cell.get('outputs', [])
             actual_outputs = actual_cell.get('outputs', [])
             normalized_expected_outputs = list(map(self.normalize_output, expected_outputs))
             normalized_actual_outputs = list(map(self.normalize_output, actual_outputs))
-            assert normalized_expected_outputs == normalized_actual_outputs
+            self.assertEqual(normalized_expected_outputs, normalized_actual_outputs)
 
             expected_execution_count = expected_cell.get('execution_count', None)
             actual_execution_count = actual_cell.get('execution_count', None)
-            assert expected_execution_count == actual_execution_count
+            self.assertEqual(expected_execution_count, actual_execution_count)
 
 
     def build_preprocessor(self):

IPython/nbconvert/preprocessors/tests/test_extractoutput.py

@@ -10,7 +10,6 b' from ..extractoutput import ExtractOutputPreprocessor'
 class TestExtractOutput(PreprocessorTestsBase):
     """Contains test functions for extractoutput.py"""
 
-
     def build_preprocessor(self):
         """Make an instance of a preprocessor"""
         preprocessor = ExtractOutputPreprocessor()
@@ -18,11 +17,9 b' class TestExtractOutput(PreprocessorTestsBase):'
         preprocessor.enabled = True
         return preprocessor
 
-
     def test_constructor(self):
         """Can a ExtractOutputPreprocessor be constructed?"""
         self.build_preprocessor()
-    
 
     def test_output(self):
         """Test the output of the ExtractOutputPreprocessor"""
@@ -32,30 +29,30 b' class TestExtractOutput(PreprocessorTestsBase):'
         nb, res = preprocessor(nb, res)
         # Check if text was extracted.
         output = nb.cells[0].outputs[1]
-        assert 'filenames' in output.metadata
-        assert 'text/plain' in output.metadata.filenames
+        self.assertIn('filenames', output.metadata)
+        self.assertIn('text/plain', output.metadata.filenames)
         text_filename = output.metadata.filenames['text/plain']
 
         # Check if png was extracted.
         output = nb.cells[0].outputs[6]
-        assert 'filenames' in output.metadata
-        assert 'image/png' in output.metadata.filenames
+        self.assertIn('filenames', output.metadata)
+        self.assertIn('image/png', output.metadata.filenames)
         png_filename = output.metadata.filenames['image/png']
         
         # Check that pdf was extracted
         output = nb.cells[0].outputs[7]
-        assert 'filenames' in output.metadata
-        assert 'application/pdf' in output.metadata.filenames
+        self.assertIn('filenames', output.metadata)
+        self.assertIn('application/pdf', output.metadata.filenames)
         pdf_filename = output.metadata.filenames['application/pdf']
 
         # Verify text output
-        assert text_filename in res['outputs']
+        self.assertIn(text_filename, res['outputs'])
         self.assertEqual(res['outputs'][text_filename], b'b')
 
         # Verify png output
-        assert png_filename in res['outputs']
+        self.assertIn(png_filename, res['outputs'])
         self.assertEqual(res['outputs'][png_filename], b'g')
 
         # Verify pdf output
-        assert pdf_filename in res['outputs']
+        self.assertIn(pdf_filename, res['outputs'])
         self.assertEqual(res['outputs'][pdf_filename], b'h')

IPython/nbconvert/preprocessors/tests/test_svg2pdf.py

@@ -39,10 +39,10 b' class Testsvg2pdf(PreprocessorTestsBase):'
 </svg>"""
 
     def build_notebook(self):
-        """Build a reveal slides notebook in memory for use with tests.  
+        """Build a reveal slides notebook in memory for use with tests.
         Overrides base in PreprocessorTestsBase"""
 
-        outputs = [nbformat.new_output(output_type="svg", output_svg=self.simple_svg)]
+        outputs = [nbformat.new_output(output_type="image/svg+xml", output_svg=self.simple_svg)]
         
         slide_metadata = {'slideshow' : {'slide_type': 'slide'}}
         subslide_metadata = {'slideshow' : {'slide_type': 'subslide'}}
@@ -71,4 +71,4 b' class Testsvg2pdf(PreprocessorTestsBase):'
         res = self.build_resources()
         preprocessor = self.build_preprocessor()
         nb, res = preprocessor(nb, res)
-        assert 'svg' in nb.cells[0].outputs[0]
+        self.assertIn('application/pdf', nb.cells[0].outputs[0])

IPython/nbformat/convert.py

@@ -44,8 +44,7 b' def convert(nb, to_version):'
         # Convert and make sure version changed during conversion.
         converted = convert_function(nb)
         if converted.get('nbformat', 1) == version:
-            raise ValueError("Cannot convert notebook from v%d to v%d.  Operation" \
-                "failed silently." % (version, step_version))
+            raise ValueError("Failed to convert notebook from v%d to v%d." % (version, step_version))
 
         # Recursively convert until target version is reached.
         return convert(converted, to_version)

IPython/nbformat/sign.py

@@ -180,9 +180,6 b' class NotebookNotary(LoggingConfigurable):'
         
         This function is the inverse of check_cells
         """
-        if not nb['cells']:
-            # nothing to mark if there are no cells
-            return
         for cell in nb['cells']:
             if cell['cell_type'] == 'code':
                 cell['metadata']['trusted'] = trusted
@@ -224,8 +221,6 b' class NotebookNotary(LoggingConfigurable):'
         
         This function is the inverse of mark_cells.
         """
-        if not nb['cells']:
-            return True
         trusted = True
         for cell in nb['cells']:
             if cell['cell_type'] != 'code':

IPython/nbformat/v4/nbbase.py

@@ -28,10 +28,7 b' class NotebookNode(Struct):'
 
 def from_dict(d):
     if isinstance(d, dict):
-        newd = NotebookNode()
-        for k,v in d.items():
-            newd[k] = from_dict(v)
-        return newd
+        return NotebookNode({k:from_dict(v) for k,v in d.items()})
     elif isinstance(d, (tuple, list)):
         return [from_dict(i) for i in d]
     else:
@@ -41,17 +38,19 b' def from_dict(d):'
 def new_output(output_type, data=None, **kwargs):
     """Create a new output, to go in the ``cell.outputs`` list of a code cell."""
     output = NotebookNode(output_type=output_type)
-    output.update(from_dict(kwargs))
-    if data is not None:
-        output.data = from_dict(data)
 
     # populate defaults:
     if output_type == 'stream':
-        output.setdefault('name', 'stdout')
-        output.setdefault('text', '')
+        output.name = u'stdout'
+        output.text = u''
     elif output_type in {'execute_result', 'display_data'}:
-        output.setdefault('metadata', NotebookNode())
-        output.setdefault('data', NotebookNode())
+        output.metadata = NotebookNode()
+        output.data = NotebookNode()
+    # load from args:
+    output.update(from_dict(kwargs))
+    if data is not None:
+        output.data = from_dict(data)
+    # validate
     validate(output, output_type)
     return output
 
@@ -79,7 +78,6 b' def output_from_msg(msg):'
             data=content['data'],
             execution_count=content['execution_count'],
         )
-
     elif msg_type == 'stream':
         return new_output(output_type=msg_type,
             name=content['name'],
@@ -102,40 +100,50 b' def output_from_msg(msg):'
 
 def new_code_cell(source='', **kwargs):
     """Create a new code cell"""
-    cell = NotebookNode(cell_type='code', source=source)
+    cell = NotebookNode(
+        cell_type='code',
+        metadata=NotebookNode(),
+        execution_count=None,
+        source=source,
+        outputs=[],
+    )
     cell.update(from_dict(kwargs))
-    cell.setdefault('metadata', NotebookNode())
-    cell.setdefault('source', '')
-    cell.setdefault('execution_count', None)
-    cell.setdefault('outputs', [])
 
     validate(cell, 'code_cell')
     return cell
 
 def new_markdown_cell(source='', **kwargs):
     """Create a new markdown cell"""
-    cell = NotebookNode(cell_type='markdown', source=source)
+    cell = NotebookNode(
+        cell_type='markdown',
+        source=source,
+        metadata=NotebookNode(),
+    )
     cell.update(from_dict(kwargs))
-    cell.setdefault('metadata', NotebookNode())
 
     validate(cell, 'markdown_cell')
     return cell
 
 def new_raw_cell(source='', **kwargs):
     """Create a new raw cell"""
-    cell = NotebookNode(cell_type='raw', source=source)
+    cell = NotebookNode(
+        cell_type='raw',
+        source=source,
+        metadata=NotebookNode(),
+    )
     cell.update(from_dict(kwargs))
-    cell.setdefault('metadata', NotebookNode())
 
     validate(cell, 'raw_cell')
     return cell
 
 def new_notebook(**kwargs):
     """Create a new notebook"""
-    nb = from_dict(kwargs)
-    nb.nbformat = nbformat
-    nb.nbformat_minor = nbformat_minor
-    nb.setdefault('cells', [])
-    nb.setdefault('metadata', NotebookNode())
+    nb = NotebookNode(
+        nbformat=nbformat,
+        nbformat_minor=nbformat_minor,
+        metadata=NotebookNode(),
+        cells=[],
+    )
+    nb.update(from_dict(kwargs))
     validate(nb)
     return nb

IPython/nbformat/v4/rwbase.py

@@ -3,32 +3,7 b''
 # Copyright (c) IPython Development Team.
 # Distributed under the terms of the Modified BSD License.
 
-from IPython.utils import py3compat
-from IPython.utils.py3compat import unicode_type, string_types
-
-# output keys that are likely to have multiline values
-_multiline_outputs = {
-    'text/plain',
-    'text/html',
-    'image/svg+xml',
-    'text/latex',
-    'application/javascript',
-}
-
-
-# FIXME: workaround for old splitlines()
-def _join_lines(lines):
-    """join lines that have been written by splitlines()
-
-    Has logic to protect against `splitlines()`, which
-    should have been `splitlines(True)`
-    """
-    if lines and lines[0].endswith(('\n', '\r')):
-        # created by splitlines(True)
-        return u''.join(lines)
-    else:
-        # created by splitlines()
-        return u'\n'.join(lines)
+from IPython.utils.py3compat import string_types, cast_unicode_py2
 
 
 def rejoin_lines(nb):
@@ -43,17 +18,17 b' def rejoin_lines(nb):'
     """
     for cell in nb.cells:
         if 'source' in cell and isinstance(cell.source, list):
-            cell.source = _join_lines(cell.source)
+            cell.source = ''.join(cell.source)
         if cell.get('cell_type', None) == 'code':
             for output in cell.get('outputs', []):
                 output_type = output.get('output_type', '')
                 if output_type in {'execute_result', 'display_data'}:
                     for key, value in output.get('data', {}).items():
                         if key != 'application/json' and isinstance(value, list):
-                            output.data[key] = _join_lines(value)
+                            output.data[key] = ''.join(value)
                 elif output_type:
                     if isinstance(output.get('text', ''), list):
-                        output.text = _join_lines(output.text)
+                        output.text = ''.join(output.text)
     return nb
 
 
@@ -103,9 +78,7 b' class NotebookReader(object):'
 
     def read(self, fp, **kwargs):
         """Read a notebook from a file like object"""
-        nbs = fp.read()
-        if not py3compat.PY3 and not isinstance(nbs, unicode_type):
-            nbs = py3compat.str_to_unicode(nbs)
+        nbs = cast_unicode_py2(fp.read())
         return self.reads(nbs, **kwargs)
 
 
@@ -118,8 +91,5 b' class NotebookWriter(object):'
 
     def write(self, nb, fp, **kwargs):
         """Write a notebook to a file like object"""
-        nbs = self.writes(nb,**kwargs)
-        if not py3compat.PY3 and not isinstance(nbs, unicode_type):
-            # this branch is likely only taken for JSON on Python 2
-            nbs = py3compat.str_to_unicode(nbs)
+        nbs = cast_unicode_py2(self.writes(nb, **kwargs))
         return fp.write(nbs)

docs/source/notebook/nbformat.rst

@@ -49,6 +49,14 b' At the highest level, a Jupyter notebook is a dictionary with a few keys:'
       ],
     }
 
+Some fields, such as code input and text output, are characteristically multi-line strings.
+When these fields are written to disk, they **may** be written as a list of strings,
+which should be joined with ``''`` when reading back into memory.
+In programmatic APIs for working with notebooks (Python, Javascript),
+these are always re-joined into the original multi-line string.
+If you intend to work with notebook files directly,
+you must allow multi-line string fields to be either a string or list of strings.
+
 
 Cell Types
 ==========
@@ -82,7 +90,7 b' as defined in `GitHub-flavored markdown`_, and implemented in marked_.'
       "source" : ["some *markdown*"],
     }
 
-.. versionchanged:: 4.0
+.. versionchanged:: nbformat 4.0
 
     Heading cells have been removed, in favor of simple headings in markdown.
 
@@ -91,8 +99,8 b' Code cells'
 ----------
 
 Code cells are the primary content of Jupyter notebooks.
-They contain source code int e language of the document's associated kernel,
-and a list of outputs associated with executing.
+They contain source code in the language of the document's associated kernel,
+and a list of outputs associated with executing that code.
 They also have an execution_count, which must be an integer or ``null``.
 
 .. sourcecode:: python
@@ -112,11 +120,11 b' They also have an execution_count, which must be an integer or ``null``.'
       }],
     }
 
-.. versionchanged:: 4.0
+.. versionchanged:: nbformat 4.0
 
     ``input`` was renamed to ``source``, for consistency among cell types.
 
-.. versionchanged:: 4.0
+.. versionchanged:: nbformat 4.0
 
     ``prompt_number`` renamed to ``execution_count``
 
@@ -141,7 +149,7 b' stream output'
       "data" : ["multiline stream text"],
     }
 
-.. versionchanged:: 4.0
+.. versionchanged:: nbformat 4.0
 
     The keys ``stream`` and ``text`` were changed to ``name`` and ``data`` to match
     the stream message specification.
@@ -176,11 +184,11 b' The metadata of these messages may be keyed by mime-type as well.'
     }
 
 
-.. versionchanged:: 4.0
+.. versionchanged:: nbformat 4.0
 
     ``application/json`` output is no longer double-serialized into a string.
 
-.. versionchanged:: 4.0
+.. versionchanged:: nbformat 4.0
 
     mime-types are used for keys, instead of a combination of short names (``text``)
     and mime-types, and are stored in a ``data`` key, rather than the top-level.
@@ -193,13 +201,13 b' execute_result'
 Results of executing a cell (as created by ``displayhook`` in Python)
 are stored in ``execute_result`` outputs.
 `execute_result` outputs are identical to ``display_data``,
-adding only a ``prompt_number`` field, which must be an integer.
+adding only a ``execution_count`` field, which must be an integer.
 
 .. sourcecode:: python
 
     {
       "output_type" : "execute_result",
-      "execute_result": 42,
+      "execution_count": 42,
       "data" : {
         "text/plain" : ["multiline text data"],
         "image/png": ["base64-encoded-png-data"],
@@ -216,11 +224,11 b' adding only a ``prompt_number`` field, which must be an integer.'
       },
     }
 
-.. versionchanged:: 4.0
+.. versionchanged:: nbformat 4.0
 
     ``pyout`` renamed to ``execute_result``
 
-.. versionchanged:: 4.0
+.. versionchanged:: nbformat 4.0
 
     ``prompt_number`` renamed to ``execution_count``
 
@@ -241,7 +249,7 b' Failed execution may show a traceback'
       'traceback' : list,
     }
 
-.. versionchanged:: 4.0
+.. versionchanged:: nbformat 4.0
 
     ``pyerr`` renamed to ``error``
 
@@ -295,8 +303,8 b' The following metadata keys are defined at the notebook level:'
 =========== =============== ==============
 Key         Value           Interpretation
 =========== =============== ==============
-kernelspec  dict            A kernel specification. More detail :ref:`here <kernelspecs>`
-signature   str             A hashed signature of the notebook
+kernelspec  dict            A :ref:`kernel specification <kernelspecs>`
+signature   str             A hashed :ref:`signature <notebook_security>` of the notebook
 =========== =============== ==============