[MRG] Notebook CLI by Titan-C · Pull Request #148 · sphinx-gallery/sphinx-gallery

Titan-C · 2016-09-24T15:09:45Z

This is a command line utility to expose sphinx gallery notebook generator.
I manually tested it on bash and zsh.

calling

sphx_ex2nb.py *.py

will convert all python files into notebooks

Titan-C · 2016-10-10T09:25:43Z

@GaelVaroquaux Any comments on this one?

lesteve · 2016-11-08T15:38:44Z

Just curious, are you using the sphx_ex2nb.py script yourself?

lesteve · 2016-11-15T12:46:41Z

setup.py

    package_data={'sphinx_gallery': ['_static/gallery.css', '_static/no_image.png',
                                     '_static/broken_example.png']},
-    scripts=['bin/copy_sphinxgallery.sh'],
+    scripts=['bin/copy_sphinxgallery.sh', 'bin/sphx_ex2nb.py'],


Maybe something less cryptic like sphx_glr_example_to_notebook? Not 100% convinced so better suggestions welcome! My point is that trying to shorten names as much as possible does not make too much sense since you have tab-completion these days ...

lesteve · 2016-11-15T12:52:27Z

bin/sphx_ex2nb.py

+    args = parser.parse_args()
+
+    for src_file in args.file:
+        file_name = os.path.basename(src_file)


For consistency I think this should be src_filename and filename. I tend to use filename for file names and file for object files (i.e. as returned from open) to make it easier to differentiate between the two.

lesteve · 2016-11-15T12:57:17Z

sphinx_gallery/notebook.py

        self.work_notebook = ipy_notebook_skeleton()
        self.add_code_cell("%matplotlib inline")
+        self.fill_notebook(script_blocks)
+        self.save_file()


It's generally considered best practice to have minimal logic in the constructor. In this case I would rather have an API like:

nb = Notebook(source_filename) nb.to_notebook(target_filename)

The parsing of the blocks from source_filename could happen in the constructor.

Titan-C · 2016-11-19T23:57:53Z

Comments addressed. Notebook generation is simpler now as well. And I have included test for the command line utility

lesteve

A few comments:

lesteve · 2016-11-22T10:17:17Z

sphinx_gallery/notebook.py


-    def __init__(self, file_name, target_dir):
-        """Declare the skeleton of the notebook
+    work_notebook = ipy_notebook_skeleton()


ipy_notebook -> jupyter_notebook here and everywhere else.

lesteve · 2016-11-22T10:20:10Z

sphinx_gallery/tests/test_notebook.py

+    """Test that written ipython notebook file corresponds to python object"""
+    blocks = sg.split_code_and_text_blocks('tutorials/plot_parse.py')
+    example_nb = jupyter_notebook(blocks)
+    nb_filename = "/tmp/testnb.ipynb"


This is not going to work on Windows. Use tempfile.NamedTemporaryFile

lesteve · 2016-11-22T10:21:28Z

bin/test_cli.py

+    FileNotFoundError = IOError
+
+
+class CommandLineTest(TestCase):


What's the point of using a test class with methods rather than just functions?

I need this class with methods because of unittest. This methods are specific to test exceptions are raised, and come inside unittest.TestCase.
To use functions I need to explicitly rely on nosetest or pytest own functions to test exceptions.

lesteve · 2016-11-22T12:40:13Z

doc/utils.rst

+Convert Python scripts into Jupyter Notebooks
+=============================================
+
+Sphinx Gallery exposes its python source to Jupyter notebook convert


lesteve · 2016-11-22T15:24:27Z

.travis.yml

 script:
    - if [ "$DISTRIB" == "ubuntu" ]; then python setup.py nosetests; fi
    - if [ "$DISTRIB" == "conda" ]; then nosetests; fi
+    - nosetests bin # to test jupyter notebook generator CLI


not very conventional ... maybe it would be great if the scripts in bin, was something super simple like:

from sphinx_gallery.notebook import main if __name__ == '__main__': main()

This way you can still test the parser in sphinx_gallery/tests.

I put this line because I could not get nosetest to scan the bin folder. Pytest did not have issues on that. But indeed I'll move everything to the notebook parsing file.

text2string into notebook to avoid circular dependence

- Converter script takes multiple files & wildcards - Install notebook converter script with setup.py

Includes the script converter and moves the gallery downloader

- Jupyter notebook generator function name is explicit

CLI inside notebook file, this allows for testing directly on the python module responsible for the rendering of the notebooks.

Titan-C · 2016-11-27T14:04:51Z

Rebased to master and comments addressed. Cleaned the commit history too

lesteve · 2016-11-27T20:00:28Z

sphinx_gallery/tests/test_notebook.py

+class CommandLineTest(TestCase):
+    """Test the Sphinx-Gallery python to Jupyter notebook converter CLI"""
+
+    def test_with_empty_args(self):


why still the class methods rather than functions?

I push a change in your branch because apart from this it is good for merging I think.

lesteve · 2016-11-27T21:31:48Z

Merging this one, thanks!

Titan-C · 2016-11-27T21:32:10Z

Thanks for the changes. I'm not sure if you got the comment about using the class methods for test. My idea behind it is that that is how unittest does in the standard library tests for exceptions. And since we plan to move to pytest. I did not wanted to include more use to nosetest.

lesteve · 2016-11-27T21:40:32Z

Oops sorry, I missed your reply somehow. In order not to have camelCase in our tests I would either the same method as in scikit-learn so that we can have from sphinx_gallery.testing import assert_raises_regex or it seems like pytest has its own way of checking exception message (from http://doc.pytest.org/en/latest/assert.html):

def test_match():
    with pytest.raises(ValueError) as excinfo:
        myfunc()
    excinfo.match(r'.* 123 .*')

lesteve · 2016-11-28T13:43:37Z

Cleaned the commit history too

Don't worry about that too much, to be honest I tend to use the "Squash and merge" button these days and edit the commit message if there were too many commits and/or too many useless commit messages.

Titan-C added the enhancement label Oct 8, 2016

Titan-C changed the title ~~Notebook CLI~~ [MRG] Notebook CLI Oct 8, 2016

Titan-C force-pushed the notebookcli branch from e3e2d87 to 67db005 Compare November 7, 2016 15:13

Titan-C mentioned this pull request Nov 7, 2016

[MRG] Large codebase refactoring #160

Closed

lesteve reviewed Nov 15, 2016

View reviewed changes

Titan-C force-pushed the notebookcli branch from d711d71 to 2d0fb66 Compare November 19, 2016 23:53

Titan-C force-pushed the notebookcli branch from 2d0fb66 to 0d28919 Compare November 20, 2016 00:02

lesteve reviewed Nov 22, 2016

View reviewed changes

Titan-C force-pushed the notebookcli branch 2 times, most recently from 64276d9 to 5fb17e3 Compare November 27, 2016 13:35

Titan-C added 7 commits November 27, 2016 14:48

Fill notebook function

24c4e9a

text2string into notebook to avoid circular dependence

Move and pass notebook test

e099edd

Python to Jupyter converter script

194aadb

- Converter script takes multiple files & wildcards - Install notebook converter script with setup.py

Documentation on Utilities

1948103

Includes the script converter and moves the gallery downloader

Reduced function set to save the notebook

fc78c94

Refactor and test notebook generation

5ff36b6

- Jupyter notebook generator function name is explicit

Test CLI script to notebook

c881f3d

CLI inside notebook file, this allows for testing directly on the python module responsible for the rendering of the notebooks.

Titan-C force-pushed the notebookcli branch from 5fb17e3 to c881f3d Compare November 27, 2016 14:00

lesteve reviewed Nov 27, 2016

View reviewed changes

lesteve added 2 commits November 27, 2016 22:17

Use functions for tests rather than class methods

f867d8e

Fix typo

1ebde7e

lesteve merged commit f1e089f into sphinx-gallery:master Nov 27, 2016

Titan-C deleted the notebookcli branch December 13, 2016 10:32

Titan-C mentioned this pull request Jan 3, 2017

[MRG] Remove nonfunctional text2string function #191

Merged

Conversation

Titan-C commented Sep 24, 2016

Uh oh!

Titan-C commented Oct 10, 2016

Uh oh!

lesteve commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Titan-C commented Nov 19, 2016

Uh oh!

lesteve left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Titan-C commented Nov 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesteve commented Nov 27, 2016

Uh oh!

Titan-C commented Nov 27, 2016

Uh oh!

lesteve commented Nov 27, 2016

Uh oh!

lesteve commented Nov 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lesteve commented Nov 8, 2016 •

edited

Loading

lesteve commented Nov 28, 2016 •

edited

Loading